tanmayrauth commented on code in PR #1558:
URL: https://github.com/apache/iceberg-go/pull/1558#discussion_r3660000719
##########
visitors.go:
##########
@@ -496,27 +504,92 @@ 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) {
Review Comment:
This decoder duplicates the initial-default decode logic in
table/arrow_utils.go:822 (defaultToScalar) — base64 for binary/fixed,
Decimal/string for decimal, etc. Both paths must decode a given initial-default
to the same value: this one feeds the filter, defaultToScalar feeds the column
the scan materializes. If they ever diverge (e.g. one switches binary from
base64 to hex), a filter on a missing-column-with-default silently returns
wrong rows — matching rows pruned, or non-matching rows returned — with no
error. They agree today, but there's no shared helper and no test pinning the
equivalence. A full dedupe is awkward since `table` imports `iceberg` and not
the reverse, so a consistency test (for each type, assert
initialDefaultLiteral(field).Any() equals what defaultToScalar produces) is the
cheap way to lock it in.
##########
table/scanner_internal_test.go:
##########
@@ -969,3 +969,72 @@ func TestProjectionV3SchemaAlreadyHasRowID(t *testing.T) {
}
})
}
+
Review Comment:
This end-to-end scan test only covers an Int32 default. The
binary/decimal/uuid cases live in
TestTranslateColumnNamesMissingFieldInitialDefault, which stops at the
TranslateColumnNames layer and never runs the scan — so the filter-vs-fill
consistency (the failure mode above) isn't verified end-to-end for exactly the
types most prone to diverge. Adding a binary and a decimal column here would
exercise both the filter path and defaultToScalar in the same test and guard
against silent divergence.
##########
visitors_test.go:
##########
@@ -233,6 +233,119 @@ func TestAlwaysExprBinding(t *testing.T) {
}
}
+func TestTranslateColumnNamesMissingFieldInitialDefault(t *testing.T) {
Review Comment:
No coverage for date/time/timestamp defaults, which take the json.Marshal →
decodeValue path (visitors.go:550). A default written by iceberg-go arrives
numeric and resolves via Int64Literal.To(DateType); one written by another
engine arrives as an ISO string ("2017-11-16") and resolves via
StringLiteral.To(DateType). Both are realistic and neither is tested — a
temporal default that decodes wrong would mis-prune old files. Worth a date or
timestamp case here.
##########
visitors.go:
##########
@@ -496,27 +504,92 @@ 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, GeographyType, GeometryType:
+ var data []byte
+ switch val := field.InitialDefault.(type) {
+ case []byte:
+ data = val
+ case string:
+ var err error
+ data, err = base64.StdEncoding.DecodeString(val)
+ if err != nil {
+ return nil, err
+ }
+ default:
+ return nil, fmt.Errorf("%w: invalid %s initial default
%v",
+ ErrInvalidSchema, field.Type,
field.InitialDefault)
+ }
+
+ switch field.Type.(type) {
+ case GeographyType, GeometryType:
+ return LiteralFromBytes(field.Type, data)
+ default:
+ return BinaryLiteral(data).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()
+ if field.InitialDefault == nil {
+ if pred.Op() == OpIsNull {
+ return AlwaysTrue{}
+ }
+
+ return AlwaysFalse{}
+ }
+
+ eval, err := ExpressionEvaluator(NewSchema(0, field),
+ unbindPredicate(pred, Reference(field.Name)), true)
+ if err != nil {
+ panic(err)
+ }
+
+ lit, err := initialDefaultLiteral(field)
Review Comment:
When a default fails to decode, this panics with a bare, context-free error
(same for the ExpressionEvaluator panic above and the eval panic below) —
VisitExpr recovers it into the error that aborts the scan at
arrow_scanner.go:734, so the user sees a raw decode failure with no hint which
column or default triggered it. Wrap it with the field, e.g.
panic(fmt.Errorf("initial-default for column %q (id %d): %w", field.Name,
field.ID, err)), and add the same context to the bare returns in
initialDefaultLiteral (the base64, json.Marshal, and decodeValue paths).
Aborting on an undecodable default is the right behavior — this is just about
making the failure diagnosable.
--
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]
