laskoviymishka commented on code in PR #1618:
URL: https://github.com/apache/iceberg-go/pull/1618#discussion_r3719189794
##########
codec/file_scan_task.go:
##########
@@ -162,6 +168,14 @@ func DecodeFileScanTask(data []byte, spec
iceberg.PartitionSpec, schema *iceberg
}, nil
}
+func validateScanRange(start, length, fileSize int64) error {
+ if start > fileSize || length > fileSize-start {
Review Comment:
Could we add a one-line note on why this is written as a subtraction rather
than `start + length > fileSize`? The `||` short-circuit is load-bearing here -
`fileSize - start` only runs once `start <= fileSize` (leaning on the caller's
`start >= 0` guard), which dodges both the underflow and the `start + length`
overflow. Without a comment it reads as trivially simplifiable, and nothing in
the tests catches that rewrite today (see the overflow case above).
##########
codec/file_scan_task.go:
##########
@@ -132,6 +135,9 @@ func DecodeFileScanTask(data []byte, spec
iceberg.PartitionSpec, schema *iceberg
if err != nil {
return table.FileScanTask{}, fmt.Errorf("codec:
DecodeFileScanTask: file: %w", err)
}
+ if err := validateScanRange(envelope.Start, envelope.Length,
file.FileSizeBytes()); err != nil {
Review Comment:
I'm a little torn on the decode-time guard. For any task we encoded
ourselves it's tautological - `EncodeFileScanTask` already validated against
the same `FileSizeBytes()` that gets baked into the DataFile blob, so this can
only ever fire on a foreign encoder.
That's the part I'd think about for #1178: once REST scan planning lands, a
remote Java/PyIceberg planner could hand us a task where `length` and the
manifest `file_size_in_bytes` came from different sources, and we'd reject
something the caller legitimately wants. No other client enforces this at
decode, and the spec doesn't bound start/length either.
Not blocking, and I'd keep the encode-time check regardless as an assertion
on our own output. But I'd either drop a comment noting decode is a defensive
coherence guard (not spec-mandated) or consider demoting it to a warning. wdyt?
##########
codec/file_scan_task_internal_test.go:
##########
@@ -110,3 +135,19 @@ func TestDecodeFileScanTaskRejectsNegativeScanRanges(t
*testing.T) {
require.Contains(t, err.Error(), "length must be non-negative")
})
}
+
+func TestDecodeFileScanTaskRejectsRangeBeyondFileSize(t *testing.T) {
+ spec := *iceberg.UnpartitionedSpec
+ builder, err := iceberg.NewDataFileBuilder(spec,
iceberg.EntryContentData,
+ "data.parquet", iceberg.ParquetFile, nil, nil, nil, 1, 100)
+ require.NoError(t, err)
+ file, err := EncodeDataFile(builder.Build(), spec, nil, 2)
+ require.NoError(t, err)
+ envelope, err := fileScanTaskSchema.Encode(&fileScanTaskEnvelope{
+ File: file, Start: 90, Length: 11,
+ })
+ require.NoError(t, err)
+
+ _, err = DecodeFileScanTask(envelope, spec, nil, 2)
+ require.ErrorContains(t, err, "scan range start=90 length=11 exceeds
file size 100")
Review Comment:
minor: this pins the full formatted message including the numbers, while
everything else in the suite (and the encode-side test) matches the looser
`"exceeds file size"`. If we ever tweak the format string this fails for no
real reason - I'd loosen it to `require.ErrorContains(t, err, "exceeds file
size")`.
##########
codec/file_scan_task_internal_test.go:
##########
@@ -19,12 +19,37 @@ package codec
import (
"encoding/binary"
+ "math"
"testing"
"github.com/apache/iceberg-go"
"github.com/stretchr/testify/require"
)
+func TestValidateScanRange(t *testing.T) {
+ for _, tt := range []struct {
+ name string
+ start, length int64
+ fileSize int64
+ shouldReturnError bool
+ }{
+ {name: "full file", length: 100, fileSize: 100},
+ {name: "empty range at EOF", start: 100, fileSize: 100},
+ {name: "start after EOF", start: 101, fileSize: 100,
shouldReturnError: true},
+ {name: "end after EOF", start: 99, length: 2, fileSize: 100,
shouldReturnError: true},
+ {name: "overflowing end", start: math.MaxInt64, length: 1,
fileSize: 100, shouldReturnError: true},
Review Comment:
this case doesn't actually exercise the overflow-safe branch.
`start=math.MaxInt64` trips `start > fileSize` first, so the `length >
fileSize-start` subtraction never runs - which is the whole reason we wrote it
as a subtraction instead of `start+length > fileSize`.
If someone "simplified" the helper to `start+length > fileSize`, this test
would still pass (`MaxInt64+1` wraps negative, and `start > fileSize` still
fires on the first clause). The case that actually pins the subtraction form is
one where the sum overflows but `start <= fileSize` holds:
```go
{name: "start+length overflows int64", start: math.MaxInt64 - 1, length: 2,
fileSize: math.MaxInt64, shouldReturnError: true},
```
There, naive `start+length` wraps to a negative and passes, while the
subtraction form correctly rejects. Same gap in the `overflowing range` case
over in `file_scan_task_test.go`. wdyt?
--
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]
