You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/23 11:27:53 UTC
[PR] fix(table): reject null position deletes [iceberg-go]
fallintoplace opened a new pull request, #1518:
URL: https://github.com/apache/iceberg-go/pull/1518
## Motivation
The Iceberg position delete schema requires `pos` values to be non-null. The Arrow scanner currently reads the raw `Int64Values` backing slice without checking the validity bitmap, so a malformed delete file containing a null position may be interpreted as a real row position instead of returning an invalid-schema error.
## Changes
- Reject null values in position delete `pos` chunks with `iceberg.ErrInvalidSchema`.
- Add a regression test covering a nullable position array and verifying the error contract.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): reject null position deletes [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1518:
URL: https://github.com/apache/iceberg-go/pull/1518
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): reject null position deletes [iceberg-go]
Posted by "fallintoplace (via GitHub)" <gi...@apache.org>.
fallintoplace commented on code in PR #1518:
URL: https://github.com/apache/iceberg-go/pull/1518#discussion_r3646985358
##########
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:
Thank you, I have fixed these issues in [bd3ba42](https://github.com/apache/iceberg-go/pull/1518/commits/bd3ba42d1d3bb78337248701a1f1a6641d8637af).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): reject null position deletes [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org