laskoviymishka commented on code in PR #1583:
URL: https://github.com/apache/iceberg-go/pull/1583#discussion_r3681110978
##########
table/arrow_scanner_posdelete_regression_test.go:
##########
@@ -32,6 +32,39 @@ import (
"github.com/stretchr/testify/require"
)
+func TestPositionDeleteColumnIndices(t *testing.T) {
+ t.Parallel()
+
+ tests := []struct {
+ name string
+ fields []arrow.Field
+ wantErr string
+ }{
+ {name: "valid", fields: []arrow.Field{{Name: "file_path", Type:
arrow.BinaryTypes.String}, {Name: "pos", Type: arrow.PrimitiveTypes.Int64}}},
+ {name: "valid with row", fields: []arrow.Field{{Name:
"file_path", Type: arrow.BinaryTypes.String}, {Name: "pos", Type:
arrow.PrimitiveTypes.Int64}, {Name: "row", Type: arrow.BinaryTypes.String}}},
+ {name: "missing file_path", fields: []arrow.Field{{Name: "pos",
Type: arrow.PrimitiveTypes.Int64}}, wantErr: `exactly one "file_path" column,
found 0`},
+ {name: "missing pos", fields: []arrow.Field{{Name: "file_path",
Type: arrow.BinaryTypes.String}}, wantErr: `exactly one "pos" column, found 0`},
+ {name: "missing both", fields: nil, wantErr: `exactly one
"file_path" column, found 0`},
+ {name: "duplicate pos", fields: []arrow.Field{{Name:
"file_path", Type: arrow.BinaryTypes.String}, {Name: "pos", Type:
arrow.PrimitiveTypes.Int64}, {Name: "pos", Type: arrow.PrimitiveTypes.Int64}},
wantErr: `exactly one "pos" column, found 2`},
+ }
+
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ filePathIndex, posIndex, err :=
positionDeleteColumnIndices(arrow.NewSchema(test.fields, nil))
+ if test.wantErr != "" {
+ require.ErrorIs(t, err,
iceberg.ErrInvalidSchema)
+ require.ErrorContains(t, err, test.wantErr)
+
+ return
+ }
+
+ require.NoError(t, err)
+ assert.Equal(t, 0, filePathIndex)
Review Comment:
Both valid cases assert `filePathIndex == 0` / `posIndex == 1`
unconditionally, so this only really checks the fixed ordering. A case with
`pos` before `file_path` (legal for Arrow) with the expected indices
parameterized per test would prove the lookup resolves by name rather than by
position. Minor.
##########
table/arrow_scanner_posdelete_regression_test.go:
##########
@@ -32,6 +32,39 @@ import (
"github.com/stretchr/testify/require"
)
+func TestPositionDeleteColumnIndices(t *testing.T) {
+ t.Parallel()
+
+ tests := []struct {
+ name string
+ fields []arrow.Field
+ wantErr string
+ }{
+ {name: "valid", fields: []arrow.Field{{Name: "file_path", Type:
arrow.BinaryTypes.String}, {Name: "pos", Type: arrow.PrimitiveTypes.Int64}}},
+ {name: "valid with row", fields: []arrow.Field{{Name:
"file_path", Type: arrow.BinaryTypes.String}, {Name: "pos", Type:
arrow.PrimitiveTypes.Int64}, {Name: "row", Type: arrow.BinaryTypes.String}}},
+ {name: "missing file_path", fields: []arrow.Field{{Name: "pos",
Type: arrow.PrimitiveTypes.Int64}}, wantErr: `exactly one "file_path" column,
found 0`},
+ {name: "missing pos", fields: []arrow.Field{{Name: "file_path",
Type: arrow.BinaryTypes.String}}, wantErr: `exactly one "pos" column, found 0`},
+ {name: "missing both", fields: nil, wantErr: `exactly one
"file_path" column, found 0`},
+ {name: "duplicate pos", fields: []arrow.Field{{Name:
"file_path", Type: arrow.BinaryTypes.String}, {Name: "pos", Type:
arrow.PrimitiveTypes.Int64}, {Name: "pos", Type: arrow.PrimitiveTypes.Int64}},
wantErr: `exactly one "pos" column, found 2`},
+ }
+
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ filePathIndex, posIndex, err :=
positionDeleteColumnIndices(arrow.NewSchema(test.fields, nil))
Review Comment:
This is the part I'd firm up before merge. The test calls
`positionDeleteColumnIndices` directly, so it proves the helper is correct but
never exercises the panic path this PR is actually fixing — the lookup inside
`readDeletes`.
`TestReadDeletesRejectsNullPos` already has the exact infra we need
(in-memory Parquet via `pqarrow.WriteTable` + `iceio.NewMemFS`). I'd add one
end-to-end case there: write a delete file whose schema drops `file_path` (or
`pos`), call `readDeletes`, and assert `errors.Is(err, ErrInvalidSchema)`. That
locks the fix in at the real entry point — right now if the helper ever got
inlined or bypassed, nothing here would catch it.
##########
table/arrow_scanner.go:
##########
@@ -449,15 +449,42 @@ func readDeletes(ctx context.Context, fs iceio.IO,
dataFile iceberg.DataFile) (_
}
defer tbl.Release()
- filePathCol :=
tbl.Column(tbl.Schema().FieldIndices("file_path")[0]).Data()
- posCol := tbl.Column(tbl.Schema().FieldIndices("pos")[0]).Data()
+ filePathIndex, posIndex, err :=
positionDeleteColumnIndices(tbl.Schema())
+ if err != nil {
+ return nil, err
+ }
+ filePathCol := tbl.Column(filePathIndex).Data()
+ posCol := tbl.Column(posIndex).Data()
if posCol.NullN() > 0 {
return nil, fmt.Errorf("%w: null pos in position delete file",
iceberg.ErrInvalidSchema)
}
return groupPosDeletesByFilePath(ctx, filePathCol, posCol)
}
+func positionDeleteColumnIndices(schema *arrow.Schema) (int, int, error) {
+ requiredColumn := func(name string) (int, error) {
+ indices := schema.FieldIndices(name)
Review Comment:
Not blocking, and this predates your change — the old `[0]` lookup used
`FieldIndices` too.
While we're adding validation here though: `file_path` and `pos` are
reserved metadata columns with spec-assigned field IDs (2147483546 /
2147483545), and this same file already resolves lineage columns by reserved ID
via `fieldIndexByID` rather than by name. A compliant writer that keeps the
field IDs but names the columns differently would hit `ErrInvalidSchema` here
instead of being read. Switching these two lookups to `fieldIndexByID` would
make it consistent with the rest of the scanner. I'm fine leaving it name-based
for this PR — if we do, a one-line comment noting the deliberate name-over-ID
choice would help the next reader. wdyt?
##########
table/arrow_scanner_posdelete_regression_test.go:
##########
@@ -32,6 +32,39 @@ import (
"github.com/stretchr/testify/require"
)
+func TestPositionDeleteColumnIndices(t *testing.T) {
+ t.Parallel()
+
+ tests := []struct {
+ name string
+ fields []arrow.Field
+ wantErr string
+ }{
+ {name: "valid", fields: []arrow.Field{{Name: "file_path", Type:
arrow.BinaryTypes.String}, {Name: "pos", Type: arrow.PrimitiveTypes.Int64}}},
+ {name: "valid with row", fields: []arrow.Field{{Name:
"file_path", Type: arrow.BinaryTypes.String}, {Name: "pos", Type:
arrow.PrimitiveTypes.Int64}, {Name: "row", Type: arrow.BinaryTypes.String}}},
+ {name: "missing file_path", fields: []arrow.Field{{Name: "pos",
Type: arrow.PrimitiveTypes.Int64}}, wantErr: `exactly one "file_path" column,
found 0`},
+ {name: "missing pos", fields: []arrow.Field{{Name: "file_path",
Type: arrow.BinaryTypes.String}}, wantErr: `exactly one "pos" column, found 0`},
+ {name: "missing both", fields: nil, wantErr: `exactly one
"file_path" column, found 0`},
+ {name: "duplicate pos", fields: []arrow.Field{{Name:
"file_path", Type: arrow.BinaryTypes.String}, {Name: "pos", Type:
arrow.PrimitiveTypes.Int64}, {Name: "pos", Type: arrow.PrimitiveTypes.Int64}},
wantErr: `exactly one "pos" column, found 2`},
Review Comment:
`requiredColumn` handles `file_path` and `pos` symmetrically, but only the
`pos` duplicate is covered. I'd add the mirror case — `{file_path, file_path,
pos}` expecting `exactly one "file_path" column, found 2` — so both directions
are pinned.
--
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]
