mattfaltyn commented on code in PR #1558:
URL: https://github.com/apache/iceberg-go/pull/1558#discussion_r3740315741
##########
visitors.go:
##########
@@ -496,29 +503,83 @@ func (columnNameTranslator) VisitUnbound(pred
UnboundPredicate) BooleanExpressio
panic(fmt.Errorf("%w: expected bound predicate, got: %s",
ErrInvalidArgument, pred.Term()))
}
+func unbindPredicate(pred BoundPredicate, ref Reference) UnboundPredicate {
+ switch p := pred.(type) {
+ case BoundUnaryPredicate:
+ return p.AsUnbound(ref)
+ case BoundLiteralPredicate:
+ return p.AsUnbound(ref, p.Literal())
+ case BoundSetPredicate:
+ return p.AsUnbound(ref, p.Literals().Members())
+ default:
+ panic(fmt.Errorf("%w: unsupported predicate: %s",
ErrNotImplemented, pred))
+ }
+}
+
+func initialDefaultLiteral(field NestedField) (Literal, error) {
+ switch field.Type.(type) {
Review Comment:
Implemented in 97a0c39. Missing top-level geometry/geography fields with
non-null defaults now fail open to AlwaysTrue before decoding, so invalid WKT
defaults cannot abort translation. Added geometry IsNull and geography NotNull
regression cases. The spec still requires these defaults to be null; this is
conservative interoperability only.
##########
table/scanner_internal_test.go:
##########
@@ -1148,3 +1149,105 @@ func TestProjectionV3SchemaAlreadyHasRowID(t
*testing.T) {
assert.Contains(t, seen, iceberg.RowIDFieldID, "_row_id must
survive projection")
})
}
+
+func TestArrowScanFiltersMissingColumnInitialDefault(t *testing.T) {
+ tbl := buildV3TableWithRows(t,
`[{"id":1,"data":"a"},{"id":2,"data":"b"}]`)
+ decimalDefault := iceberg.Decimal{Val: decimal128.FromI64(1234), Scale:
2}
+
+ txn := tbl.NewTransaction()
+ require.NoError(t, txn.UpdateSchema(true, false).
+ AddColumn(
+ []string{"new_col"},
+ iceberg.PrimitiveTypes.Int32,
+ "",
+ false,
+ iceberg.Int32Literal(42),
+ ).
+ AddColumn(
+ []string{"new_decimal"},
+ iceberg.DecimalTypeOf(9, 2),
+ "",
+ false,
+ iceberg.DecimalLiteral(decimalDefault),
+ ).
+ Commit())
+ var err error
+ tbl, err = txn.Commit(t.Context())
+ require.NoError(t, err)
+
+ tests := []struct {
+ name string
+ filter iceberg.BooleanExpression
+ expected int64
+ }{
+ {
+ name: "matching equality",
+ filter: iceberg.EqualTo(iceberg.Reference("new_col"),
int32(42)),
+ expected: 2,
+ },
+ {
+ name: "mismatching equality",
+ filter: iceberg.EqualTo(iceberg.Reference("new_col"),
int32(7)),
+ expected: 0,
+ },
+ {
+ name: "is null",
+ filter: iceberg.IsNull(iceberg.Reference("new_col")),
+ expected: 0,
+ },
+ {
+ name: "not null",
+ filter: iceberg.NotNull(iceberg.Reference("new_col")),
+ expected: 2,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ scan := tbl.Scan(
+ WithSelectedFields("id", "new_col"),
+ WithRowFilter(tt.filter),
+ )
+ tasks, err := scan.PlanFiles(t.Context())
+ require.NoError(t, err)
+ require.Len(t, tasks, 1, "manifest planning must retain
the old file")
Review Comment:
Agreed on the limitation. I retained PlanFiles only as a stage-separation
control showing manifest planning keeps the old file; it is not the proof of
folding. The translator matrix directly asserts AlwaysTrue/AlwaysFalse, the
scan result counts exercise matching and mismatching paths, and the new binary
consistency test jointly checks translation and Arrow materialization.
##########
visitors.go:
##########
@@ -496,29 +503,83 @@ func (columnNameTranslator) VisitUnbound(pred
UnboundPredicate) BooleanExpressio
panic(fmt.Errorf("%w: expected bound predicate, got: %s",
ErrInvalidArgument, pred.Term()))
}
+func unbindPredicate(pred BoundPredicate, ref Reference) UnboundPredicate {
+ switch p := pred.(type) {
+ case BoundUnaryPredicate:
+ return p.AsUnbound(ref)
+ case BoundLiteralPredicate:
+ return p.AsUnbound(ref, p.Literal())
+ case BoundSetPredicate:
+ return p.AsUnbound(ref, p.Literals().Members())
+ default:
+ panic(fmt.Errorf("%w: unsupported predicate: %s",
ErrNotImplemented, pred))
+ }
+}
+
+func initialDefaultLiteral(field NestedField) (Literal, error) {
+ switch field.Type.(type) {
+ case BinaryType, FixedType:
+ if val, ok := field.InitialDefault.([]byte); ok {
+ return BinaryLiteral(val).To(field.Type)
+ }
+ case DecimalType:
+ if val, ok := field.InitialDefault.(Decimal); ok {
+ return DecimalLiteral(val).To(field.Type)
+ }
+ }
+
+ data, err := json.Marshal(field.InitialDefault)
+ if err != nil {
+ return nil, err
+ }
+
+ return decodeValue(data, field.Type)
+}
+
func (c columnNameTranslator) VisitBound(pred BoundPredicate)
BooleanExpression {
fileColName, found :=
c.fileSchema.FindColumnName(pred.Term().Ref().Field().ID)
if !found {
// in the case of schema evolution, the column might not be
present
// in the file schema when reading older data
- if pred.Op() == OpIsNull {
+ field := pred.Ref().Field()
+ // A nested field can still be null when an optional parent is
null, so
+ // its default is not a file-wide constant. Preserve the
existing
+ // missing-column behavior until translation has row-level
parent state.
+ if field.InitialDefault == nil || len(pred.Ref().PosPath()) > 1
{
+ if pred.Op() == OpIsNull {
+ return AlwaysTrue{}
+ }
+
+ return AlwaysFalse{}
+ }
+
+ withContext := func(err error) error {
+ return fmt.Errorf("initial-default for column %q (id
%d): %w",
+ field.Name, field.ID, err)
+ }
+ eval, err := ExpressionEvaluator(NewSchema(0, field),
+ unbindPredicate(pred, Reference(field.Name)), true)
+ if err != nil {
+ panic(withContext(err))
+ }
+
+ lit, err := initialDefaultLiteral(field)
Review Comment:
Fixed in 97a0c39. Filter translation and defaultToScalar now use the same
hex-first byte-default decoder, NestedField writes new binary/fixed defaults as
lowercase hex, and legacy v0.6.0 base64 remains readable.
TestInitialDefaultFilterProjectionConsistency verifies that the bytes admitted
by the translated predicate are exactly the bytes Arrow materializes.
##########
visitors.go:
##########
@@ -496,29 +503,83 @@ func (columnNameTranslator) VisitUnbound(pred
UnboundPredicate) BooleanExpressio
panic(fmt.Errorf("%w: expected bound predicate, got: %s",
ErrInvalidArgument, pred.Term()))
}
+func unbindPredicate(pred BoundPredicate, ref Reference) UnboundPredicate {
+ switch p := pred.(type) {
+ case BoundUnaryPredicate:
+ return p.AsUnbound(ref)
+ case BoundLiteralPredicate:
+ return p.AsUnbound(ref, p.Literal())
+ case BoundSetPredicate:
+ return p.AsUnbound(ref, p.Literals().Members())
+ default:
+ panic(fmt.Errorf("%w: unsupported predicate: %s",
ErrNotImplemented, pred))
+ }
+}
+
+func initialDefaultLiteral(field NestedField) (Literal, error) {
+ switch field.Type.(type) {
+ case BinaryType, FixedType:
+ if val, ok := field.InitialDefault.([]byte); ok {
+ return BinaryLiteral(val).To(field.Type)
+ }
+ case DecimalType:
+ if val, ok := field.InitialDefault.(Decimal); ok {
+ return DecimalLiteral(val).To(field.Type)
+ }
+ }
+
+ data, err := json.Marshal(field.InitialDefault)
+ if err != nil {
+ return nil, err
+ }
+
+ return decodeValue(data, field.Type)
+}
+
func (c columnNameTranslator) VisitBound(pred BoundPredicate)
BooleanExpression {
fileColName, found :=
c.fileSchema.FindColumnName(pred.Term().Ref().Field().ID)
if !found {
// in the case of schema evolution, the column might not be
present
// in the file schema when reading older data
- if pred.Op() == OpIsNull {
+ field := pred.Ref().Field()
+ // A nested field can still be null when an optional parent is
null, so
+ // its default is not a file-wide constant. Preserve the
existing
+ // missing-column behavior until translation has row-level
parent state.
+ if field.InitialDefault == nil || len(pred.Ref().PosPath()) > 1
{
Review Comment:
Agreed. I kept the conservative top-level-only guard unchanged in this PR.
Folding nested defaults beneath all-required ancestors is a valid optimization,
but it is intentionally left out of this correctness fix.
##########
visitors.go:
##########
@@ -496,29 +503,83 @@ func (columnNameTranslator) VisitUnbound(pred
UnboundPredicate) BooleanExpressio
panic(fmt.Errorf("%w: expected bound predicate, got: %s",
ErrInvalidArgument, pred.Term()))
}
+func unbindPredicate(pred BoundPredicate, ref Reference) UnboundPredicate {
+ switch p := pred.(type) {
+ case BoundUnaryPredicate:
+ return p.AsUnbound(ref)
+ case BoundLiteralPredicate:
+ return p.AsUnbound(ref, p.Literal())
+ case BoundSetPredicate:
+ return p.AsUnbound(ref, p.Literals().Members())
+ default:
+ panic(fmt.Errorf("%w: unsupported predicate: %s",
ErrNotImplemented, pred))
+ }
+}
+
+func initialDefaultLiteral(field NestedField) (Literal, error) {
+ switch field.Type.(type) {
+ case BinaryType, FixedType:
+ if val, ok := field.InitialDefault.([]byte); ok {
Review Comment:
Addressed in 97a0c39. Metadata binary/fixed strings now take an explicit
shared decoder path instead of relying on fallthrough. The comment records that
spec metadata is hex and that legacy iceberg-go v0.6.0 base64 remains readable;
hex wins when ambiguous.
--
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]
