You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/28 23:00:25 UTC
[PR] fix(substrait): preserve nested reference paths [iceberg-go]
fallintoplace opened a new pull request, #1588:
URL: https://github.com/apache/iceberg-go/pull/1588
## What changed
Build Substrait field references from the positional path retained by the bound Iceberg reference. Add coverage for a colliding top-level leaf name, a missing top-level leaf name, and a deeply nested field.
## Why
The converter rebound only the leaf field name. Nested references could therefore target a same-named top-level column or fail conversion when no top-level field shared that name.
## Testing
- `go test ./table/substrait`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(substrait): preserve nested reference paths [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1588:
URL: https://github.com/apache/iceberg-go/pull/1588#discussion_r3678425837
##########
table/substrait/substrait.go:
##########
@@ -348,12 +348,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:
Deleting the rebind also leaves some dead plumbing worth cleaning up in the same PR: getRef was the only reader of t.schema and t.caseSensitive, so both struct fields (lines 216-218) are now write-only, and ConvertExpr's public `caseSensitive` parameter is now entirely unused — its only consumer was this rebind, and callers already bind with case sensitivity upstream (arrow_scanner.go:748 calls iceberg.BindExpr(fileSchema, translatedFilter, as.caseSensitive) right before ConvertExpr). Net effect: a caller who flips caseSensitive on ConvertExpr expecting case-insensitive matching silently gets nothing, and go vet won't flag the write-only fields. Suggest dropping the two struct fields and the constructor args now; for the public param, either remove it (breaking, so worth a maintainer call) or keep it and document that case sensitivity is applied at bind time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(substrait): preserve nested reference paths [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1588:
URL: https://github.com/apache/iceberg-go/pull/1588
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(substrait): preserve nested reference paths [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org