laskoviymishka commented on code in PR #1518:
URL: https://github.com/apache/iceberg-go/pull/1518#discussion_r3646094561
##########
table/arrow_scanner_posdelete_regression_test.go:
##########
@@ -265,6 +265,27 @@ func
TestCollectPosDeletePositionsRejectsUnsupportedPosType(t *testing.T) {
assert.Contains(t, err.Error(), "unsupported pos column type")
}
+func TestCollectPosDeletePositionsRejectsNullPos(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ bldr := array.NewInt64Builder(mem)
+ bldr.Append(1)
+ bldr.AppendNull()
+ bldr.Append(2)
+ posArr := bldr.NewInt64Array()
+ bldr.Release()
+ defer posArr.Release()
+
+ posCol := arrow.NewChunked(arrow.PrimitiveTypes.Int64,
[]arrow.Array{posArr})
+ defer posCol.Release()
+
+ deletes, err := collectPosDeletePositions(positionDeletes{posCol})
Review Comment:
This calls `collectPosDeletePositions` directly, so it proves the guard in
isolation but never exercises the actual path through `readDeletes` →
`groupPosDeletesByFilePath`. That's exactly where the reachability question
lives, so as written the test stays green even if a null pos gets silently
dropped upstream.
A test that builds a delete table with a null-pos row and drives it through
`readDeletes` would lock the end-to-end contract and settle the Filter question
either way.
##########
table/arrow_scanner.go:
##########
@@ -408,6 +408,10 @@ func collectPosDeletePositions(positionalDeletes
positionDeletes) (set[int64], e
return nil, fmt.Errorf("%w: unsupported pos
chunk array type %T in position delete file",
iceberg.ErrInvalidSchema, arr)
}
+ if posArr.NullN() > 0 {
Review Comment:
The guard itself is correct, but I'm not sure it's reachable on the real
path. By the time we hit `collectPosDeletePositions`, `readDeletes` has already
pushed `posCol` through `groupPosDeletesByFilePath`, which runs
`compute.Filter(..., DefaultFilterOptions())` — and I'm genuinely unsure
whether arrow-go's default `NullSelectionBehavior` preserves a null-pos row
here or silently drops it before it ever reaches this loop.
Rather than hang correctness on that, I'd hoist the check up into
`readDeletes` where `posCol` is first extracted, mirroring the
`filePathCol.NullN() > 0` guard already at line 304. Then it fires before
Filter regardless of the semantics. wdyt?
##########
table/arrow_scanner_posdelete_regression_test.go:
##########
@@ -265,6 +265,27 @@ func
TestCollectPosDeletePositionsRejectsUnsupportedPosType(t *testing.T) {
assert.Contains(t, err.Error(), "unsupported pos column type")
}
+func TestCollectPosDeletePositionsRejectsNullPos(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ bldr := array.NewInt64Builder(mem)
+ bldr.Append(1)
+ bldr.AppendNull()
+ bldr.Append(2)
+ posArr := bldr.NewInt64Array()
+ bldr.Release()
+ defer posArr.Release()
+
+ posCol := arrow.NewChunked(arrow.PrimitiveTypes.Int64,
[]arrow.Array{posArr})
+ defer posCol.Release()
+
+ deletes, err := collectPosDeletePositions(positionDeletes{posCol})
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ assert.Nil(t, deletes)
+ assert.Contains(t, err.Error(), "null pos")
Review Comment:
Minor, but `"null pos"` is short enough that an unrelated future message
could match it and give a false pass. The sibling assertions all pin a longer
fragment, so I'd widen this to `"null pos in position delete file"` to match.
--
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]
