laskoviymishka commented on code in PR #1558:
URL: https://github.com/apache/iceberg-go/pull/1558#discussion_r3719291617
##########
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:
Taking geo out of here was right for the base64 problem, but nothing picks
it up now. `StringLiteral.To` has no `Geometry`/`Geography` arm, and
`convertValue` in `expr_json.go` doesn't either, so a WKT default like `"POINT
(30 10)"` goes `json.Marshal` to `decodeValue` to `ErrBadCast`, and the
recovered panic turns into a failed `TranslateColumnNames` for the whole scan.
That's a step back from where we started. Before this PR the column folded
to `AlwaysFalse`/`AlwaysTrue` and the read went through; now it errors out.
Reachability is narrow since Java's `SingleValueParser` doesn't emit geo
initial-defaults today, so it takes a non-Java V3 writer to trigger, but I'd
still not ship the hard error.
Simplest thing I see is keeping the old conservative behavior for types we
can't decode: if `initialDefaultLiteral` can't produce a literal for geo, treat
the column as "could match" instead of panicking. wdyt?
##########
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:
This does the right thing, but not for the reason it looks like.
`InitialDefault` on a JSON-loaded schema is always a Go `string`, so the
`[]byte` assertion misses and we fall through to `json.Marshal` +
`decodeValue`, which hex-decodes. That's exactly the behavior we wanted, and
the `"000102ff"` / `"010203"` cases lock it in.
My worry is the next reader. Nothing says the fallthrough is load-bearing,
so adding an `else { return error }` looks like obvious tidying and would break
every metadata-sourced binary default. A line like `// metadata defaults arrive
as hex strings; fall through to decodeValue` would keep it safe.
##########
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:
Appreciate the comment, it states the limitation clearly, and this leaves
nested fields where they already were instead of regressing them. Fine by me
for this PR.
Worth noting for later: when every ancestor is required the child default
*is* a file-wide constant, so `WHERE addr.country = 'US'` over a `REQUIRED
STRUCT addr` could fold safely and this bails out anyway. Narrowing the guard
to "bail only if some ancestor is optional" would recover that. Happy for it to
be a follow-up.
##########
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:
Circling back on the `defaultToScalar` note I called a follow-up last round:
I think it got more urgent, not less. With the filter path on hex, a
Java-written `"initial-default": "000102ff"` folds the predicate to
`AlwaysTrue` correctly here, and then `defaultToScalar` in
`table/arrow_utils.go` base64-decodes the same string to fill the column.
`000102ff` is entirely base64-alphabet characters, so it doesn't even error, it
decodes into six garbage bytes.
So the scan returns rows whose binary column doesn't match the predicate
that admitted them, with no error anywhere. Nothing this PR broke, but this PR
is what makes it observable. Your call whether converging `defaultToScalar` on
hex rides along here or gets filed, but I wouldn't let it sit.
##########
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:
I don't think this assertion can fail. When `new_col` is missing from the
file there are no stats for it, so the inclusive-metrics evaluator falls
through to `rowsMightMatch` and the file passes whether or not the fold was
right. It can't distinguish correct pruning from absent stats.
Same shape one level down: for the `expected: 0` cases the fold to
`AlwaysFalse` drops the file, `ToArrowTable` returns zero chunks, and the `==
42` loop never executes. The default-fill check is only really exercised by the
two `expected: 2` cases.
If we want the planning-level claim to bite, asserting on the translated
expression or the record filter rather than task count would do it.
--
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]
