zeroshade commented on code in PR #1128:
URL: https://github.com/apache/iceberg-go/pull/1128#discussion_r3523596673
##########
table/scanner.go:
##########
@@ -362,6 +362,17 @@ func (scan *Scan) buildPartitionProjection(specID int)
(iceberg.BooleanExpressio
return buildPartitionProjection(specID, scan.metadata, scan.rowFilter,
scan.caseSensitive)
}
+func (scan *Scan) validateRowFilter() error {
+ _, err := newInclusiveMetricsEvaluator(
+ scan.metadata.CurrentSchema(),
Review Comment:
Minor: this constructs a full `newInclusiveMetricsEvaluator` only to check
whether the filter binds, then discards it. Since non-empty planning builds the
real evaluator later anyway, please prefer validating directly with
`RewriteNotExpr` plus `BindExpr(scan.metadata.CurrentSchema(), ...)` instead of
allocating an evaluator here.
##########
table/scanner.go:
##########
@@ -657,6 +668,10 @@ func (scan *Scan) PlanFiles(ctx context.Context)
([]FileScanTask, error) {
scan.snapshotID = &snapshot.SnapshotID
scan.asOfTimestamp = nil
}
+ if err := scan.validateRowFilter(); err != nil {
+ return nil, err
Review Comment:
Question: this placement intentionally validates filters before the
empty-scan paths, which fixes #1119 for iceberg-go. It does diverge from
Java/PyIceberg/iceberg-rust behavior described in review, where empty-scan
paths can return empty results without binding filters. Please confirm the
intended cross-client behavior here: accept the stricter validation in
iceberg-go, or gate this validation so it only runs when a snapshot/manifests
exist.
--
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]
