tanmayrauth commented on code in PR #1395:
URL: https://github.com/apache/iceberg-go/pull/1395#discussion_r3525687460
##########
codec/file_scan_task_internal_test.go:
##########
@@ -47,3 +47,38 @@ func TestDecodeFileScanTaskInnerErrorCarriesMarker(t
*testing.T) {
require.Contains(t, err.Error(), "codec: DecodeFileScanTask: file:",
"an inner (primary-file) decode error must carry the function +
sub-path marker")
}
+
Review Comment:
Minor: since the range check returns before DecodeDataFile is ever reached,
the spec/schema fixtures here aren't actually exercised on this path. Fine to
leave, but you could drop them or add a one-liner noting the check
intentionally precedes file decode.
##########
codec/file_scan_task.go:
##########
@@ -87,6 +87,12 @@ func DecodeFileScanTask(data []byte, spec
iceberg.PartitionSpec, schema *iceberg
if _, err := fileScanTaskSchema.Decode(data, &envelope); err != nil {
return table.FileScanTask{}, fmt.Errorf("codec:
DecodeFileScanTask: decode: %w", err)
}
+ if envelope.Start < 0 {
Review Comment:
Nice that this sits before DecodeDataFile so a malformed envelope is
rejected cheaply. One thought: EncodeFileScanTask copies task.Start/task.Length
straight through without the same check, so a task built with a negative range
encodes fine and only blows up on decode. Since this is a cross-process
boundary, would you consider mirroring these two guards in the encoder so a bad
range fails fast at the producer too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
