laskoviymishka commented on code in PR #1588:
URL: https://github.com/apache/iceberg-go/pull/1588#discussion_r3680745221
##########
table/substrait/substrait.go:
##########
@@ -348,12 +344,7 @@ func toSubstraitLiteralSet(typ iceberg.Type, lits
[]iceberg.Literal) expr.ListLi
}
func (t *toSubstraitExpr) getRef(ref iceberg.BoundReference) expr.Reference {
- updatedRef, err := iceberg.Reference(ref.Field().Name).Bind(t.schema,
t.caseSensitive)
- if err != nil {
- panic(err)
- }
-
- path := updatedRef.Ref().PosPath()
+ path := ref.PosPath()
Review Comment:
`path[0]` here leans on `PosPath()` always returning at least one element —
true by construction today since `acc.pos` is unconditionally first, but it's
an implicit invariant. A one-line comment noting that, or a cheap `len` guard,
would save a future BoundReference impl from a bare slice-bounds panic.
Non-blocking.
##########
table/substrait/substrait.go:
##########
@@ -51,9 +51,10 @@ func NewExtensionSet() exprs.ExtensionIDSet {
return
exprs.NewExtensionSetDefault(expr.NewEmptyExtensionRegistry(collection))
}
-// ConvertExpr binds the provided expression to the given schema and converts
it to a
-// substrait expression so that it can be utilized for computation.
-func ConvertExpr(schema *iceberg.Schema, e iceberg.BooleanExpression,
caseSensitive bool) (*expr.ExtensionRegistry, expr.Expression, error) {
+// ConvertExpr converts a bound expression to a Substrait expression so that
it can
+// be utilized for computation. Case sensitivity is applied when binding; the
third
+// argument is retained for API compatibility.
+func ConvertExpr(schema *iceberg.Schema, e iceberg.BooleanExpression, _ bool)
(*expr.ExtensionRegistry, expr.Expression, error) {
Review Comment:
The doc still says "case sensitivity is applied when binding," but
ConvertExpr doesn't bind anymore — it now assumes `e` is already bound and the
third arg is dead. Harmless today since both callers pre-bind, but the comment
points future readers the wrong way; I'd reword it to say the expr must be
pre-bound and the bool is retained only for compat (probably overlaps one of
the cleanup nits tanmayrauth already flagged).
If we want to actually enforce the pre-bound precondition rather than lean
on convention, a guard at the top returning an error would beat the
`VisitUnbound` panic when someone passes an unbound expr — optional though.
wdyt?
##########
table/substrait/substrait_test.go:
##########
@@ -79,6 +79,60 @@ func TestRefTypes(t *testing.T) {
}
}
+func TestNestedReferencesPreserveBoundPath(t *testing.T) {
+ sc := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64},
+ iceberg.NestedField{ID: 2, Name: "customer", Type:
&iceberg.StructType{FieldList: []iceberg.NestedField{
+ {ID: 3, Name: "id", Type: iceberg.PrimitiveTypes.Int64},
+ {ID: 4, Name: "address", Type:
&iceberg.StructType{FieldList: []iceberg.NestedField{
+ {ID: 5, Name: "zip", Type:
iceberg.PrimitiveTypes.String},
+ }}},
+ }}},
+ )
+
+ tests := []struct {
+ name string
+ predicate iceberg.UnboundPredicate
+ expected string
+ }{
+ {
+ name: "customer.id",
+ predicate:
iceberg.EqualTo(iceberg.Reference("customer.id"), int64(123)),
+ expected: ".field(1).field(0)",
+ },
+ {
+ name: "customer.address.zip",
+ predicate:
iceberg.EqualTo(iceberg.Reference("customer.address.zip"), "12345"),
+ expected: ".field(1).field(1).field(0)",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ bound, err := tt.predicate.Bind(sc, true)
+ require.NoError(t, err)
+
+ _, converted, err := substrait.ConvertExpr(sc, bound,
true)
+ require.NoError(t, err)
+ assert.Contains(t, converted.String(), tt.expected)
Review Comment:
`assert.Contains` means a wrong path could still slip through — e.g.
`.field(0).field(1).field(0)` contains `.field(1).field(0)` as a substring and
would pass while being wrong. Since pinning the exact path is the whole point
of this regression, I'd assert the full converted string (or extract just the
field-ref chain) rather than a substring. Same goes for the
`.field(0).field(0)` check in `TestNestedReferenceWithoutTopLevelLeafName`.
--
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]
