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: [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]
