fix(codec): validate primary data file spec id in EncodeFileScanTask#1287
Conversation
Three related codec cleanups: - EncodeFileScanTask validated the SpecID of every delete/equality/DV file (via encodeDataFileSlice) but not the primary task.File. EncodeDataFile encodes partition data against the passed-in spec, so a transposed (spec, task) pair silently mis-mapped or dropped partition values and still returned success. Guard the primary file the same way the delete files are guarded, and update the doc comment to say every file is checked. - Tag the Encode/DecodeFileScanTask propagation-path errors with the "codec: EncodeFileScanTask:" / "codec: DecodeFileScanTask:" markers, matching the convention EncodeDataFile/DecodeDataFile already use (and that data_file_test.go asserts). - Fix the SetSchemaCacheSize doc: it claimed "not safe to call concurrently", but the underlying golang-lru/v2 cache serializes Resize against Get/Add through the same mutex, so it is safe.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Approve. Three correct, well-scoped changes; verified each locally.
1. Primary-file SpecID guard (the real fix). Confirmed EncodeDataFile encodes partition data against the passed-in spec and does not self-validate df.SpecID(), so a transposed (spec, task.File) pair silently produced a corrupt blob. Proved it load-bearing: removing the guard makes TestEncodeFileScanTaskRejectsMismatchedPrimaryDataFileSpec fail with "An error is expected but got nil". The checkDataFileSpecID extraction is behavior-preserving (same message; encodeDataFileSlice still wraps with entry %d:).
2. Error-marker tagging. New codec: EncodeFileScanTask: / codec: DecodeFileScanTask: markers match the EncodeDataFile/DecodeDataFile convention asserted in data_file_test.go. Nice touch using an internal test to reach the inner primary-file decode path.
3. Concurrency-doc fix. Verified against golang-lru/v2 v2.0.7: Resize, Get, and Add all take the same c.lock, so SetSchemaCacheSize is genuinely safe to call concurrently. Old comment was wrong; new one is accurate.
Verification: codec tests pass, golangci-lint clean (0 issues), gofmt/vet clean. Only production caller of EncodeDataFile is file_scan_task.go, now fully guarded.
Nit (non-blocking): the primary-file spec-guard error omits the file: sub-label the primary encode error uses, so it reads codec: EncodeFileScanTask: ... vs delete files' ... delete files: entry N: .... Wrapping the guard with file: %w would make primary-vs-delete origin symmetric. Cosmetic.
2 participants
Three related codec cleanups:
EncodeFileScanTask validated the SpecID of every delete/equality/DV file (via encodeDataFileSlice) but not the primary task.File. EncodeDataFile encodes partition data against the passed-in spec, so a transposed (spec, task) pair silently mis-mapped or dropped partition values and still returned success. Guard the primary file the same way the delete files are guarded, and update the doc comment to say every file is checked.
Tag the Encode/DecodeFileScanTask propagation-path errors with the "codec: EncodeFileScanTask:" / "codec: DecodeFileScanTask:" markers, matching the convention EncodeDataFile/DecodeDataFile already use (and that data_file_test.go asserts).
Fix the SetSchemaCacheSize doc: it claimed "not safe to call concurrently", but the underlying golang-lru/v2 cache serializes Resize against Get/Add through the same mutex, so it is safe.