laskoviymishka commented on code in PR #1347:
URL: https://github.com/apache/iceberg-go/pull/1347#discussion_r3501966057
##########
table/equality_delete_writer.go:
##########
@@ -46,10 +55,28 @@ func equalityDeleteSchema(tableSchema *iceberg.Schema,
fieldIDs []int) (*iceberg
return nil, fmt.Errorf("%w: field ID %d not found in
table schema", iceberg.ErrInvalidSchema, id)
}
+ field, ok := tableSchema.FindFieldByID(id)
Review Comment:
This lookup pair looks redundant but isn't — worth a comment so nobody
"simplifies" it.
`FindColumnName(id)` above returns the dotted path (`address.zipcode`) that
`Schema.Select` needs to project nested fields; `FindFieldByID(id).Name` only
gives the bare short name (`zipcode`), so it can't replace it. We keep
`FindColumnName` for the projection name and `FindFieldByID` for the `Type`.
Dropping the former would silently break nested-field equality deletes — a
one-liner here (and noting the `!ok` branch is unreachable after
`FindColumnName` already succeeded) prevents that.
##########
table/equality_delete_writer.go:
##########
@@ -46,10 +55,28 @@ func equalityDeleteSchema(tableSchema *iceberg.Schema,
fieldIDs []int) (*iceberg
return nil, fmt.Errorf("%w: field ID %d not found in
table schema", iceberg.ErrInvalidSchema, id)
}
+ field, ok := tableSchema.FindFieldByID(id)
+ if !ok {
+ return nil, fmt.Errorf("%w: field ID %d not found in
table schema", iceberg.ErrInvalidSchema, id)
+ }
+ if isFloatingPointType(field.Type) {
+ return nil, fmt.Errorf("%w: equality field ID %d (%s)
has unsupported type %s: float and double cannot be used in equality delete
field IDs",
Review Comment:
For a float field this reads "...has unsupported type float: float and
double cannot be used..." — the "double" in the static suffix is noise when
only float triggered. I'd tighten to something like "...has unsupported
floating-point type %s; floating-point columns cannot be used as equality
delete keys". The test asserts on the `float`/`double` substring via the
`field.Type` interpolation, so that still holds.
##########
table/row_delta.go:
##########
@@ -134,11 +134,8 @@ func (rd *RowDelta) Commit(ctx context.Context) error {
f.FilePath())
}
- for _, id := range eqIDs {
- if _, ok := schema.FindFieldByID(id); !ok {
- return fmt.Errorf("equality field ID %d
not found in table schema: %s",
- id, f.FilePath())
- }
+ if _, err := validateEqualityFieldIDs(schema, eqIDs);
err != nil {
+ return fmt.Errorf("invalid equality delete file
%s: %w", f.FilePath(), err)
Review Comment:
Good catch wrapping with `%w` — the old "not found" error was unwrapped, so
`errors.Is(err, ErrInvalidSchema)` now succeeds and the "not found in table
schema" substring is preserved. Just flagging that this commit-path wording
("invalid equality delete file %s") differs from the writer call sites, and the
two test files re-derive the expected substrings independently — if the float
message changes per the other comment, both need updating in lockstep.
##########
table/equality_delete_writer_test.go:
##########
@@ -226,6 +226,53 @@ func TestWriteEqualityDeleteFilesRejectsInvalidFieldID(t
*testing.T) {
require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
}
+func TestWriteEqualityDeleteFilesRejectsFloatAndDoubleFieldIDs(t *testing.T) {
+ location := filepath.ToSlash(t.TempDir())
+
+ iceSchema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ iceberg.NestedField{ID: 2, Name: "score", Type:
iceberg.PrimitiveTypes.Float32, Required: false},
+ iceberg.NestedField{ID: 3, Name: "ratio", Type:
iceberg.PrimitiveTypes.Float64, Required: false},
+ )
+
+ meta, err := table.NewMetadata(iceSchema, iceberg.UnpartitionedSpec,
+ table.UnsortedSortOrder, location,
+ iceberg.Properties{table.PropertyFormatVersion: "2"})
+ require.NoError(t, err)
+
+ tbl := table.New(
+ table.Identifier{"db", "floating_key_test"},
+ meta, location+"/metadata/v1.metadata.json",
+ func(ctx context.Context) (iceio.IO, error) {
+ return iceio.LocalFS{}, nil
+ },
+ &rowDeltaCatalog{metadata: meta},
+ )
+
+ records := func(yield func(arrow.RecordBatch, error) bool) {}
+
+ tests := []struct {
+ name string
+ fieldID int
+ fieldName string
+ fieldType string
+ }{
+ {name: "float", fieldID: 2, fieldName: "score", fieldType:
"float"},
Review Comment:
The most valuable test gap to close: nothing exercises
`equalityDeleteWriteSchema` (the partitioned path) with a float key end-to-end
— this unpartitioned table only hits `equalityDeleteSchema`, and the refactor
changed `equalityDeleteWriteSchema`'s shape the most. A partitioned-table case
rejecting a float key would cover it. While the scaffold's right here, a
decimal field asserted as *accepted* is a cheap negative control that pins
`isFloatingPointType`'s `default: false` so nobody later conflates "non-integer
numeric" with "floating point."
--
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]
