andygrove opened a new pull request, #6274: URL: https://github.com/apache/datafusion-comet/pull/6274
Backport of the transform residual fix in #6154 to `branch-1.0`. Hand-ported from `518584ee0d65b8e268a0c106f9c221b8f3f058f3`. Only part of #6154 is included, and the title drops ", fail on residual errors" to match; see "What changes are included" below. ## Which issue does this PR close? None. The issue #6154 links, #5992, is about the half of #6154 this backport leaves out. The bug fixed here was found while working on it and has no issue of its own. Listed in #6201. ## Rationale for this change The bug ships in 1.0.0. On `branch-1.0`, `CometIcebergNativeScan.icebergExprToProto` reads a residual predicate's column with `term.ref().name()`. On an `UnboundTransform`, `ref()` is the transform's source column, so `bucket(4, id) = 2` reaches iceberg-rust as `id = 2`. At the iceberg-rust revision `branch-1.0` pins (`3d84c81`), the reader installs the task predicate as a Parquet row filter. So rows that match the real predicate but not the misread one are dropped before the filter above the scan sees them, and that filter cannot bring them back. The comment on `icebergExprToProto` called the pushed predicate a pruning hint, which is the assumption this breaks. `CometScanRule` falls back for a bare transform predicate, but not for one under `AND`, `OR` or `NOT`. So the query returns too few rows when a system function such as `bucket`, `truncate` or `days` is combined with another predicate. That also needs Iceberg's SQL extensions, whose `ReplaceStaticInvoke` rule pushes those comparisons to the scan. The native Iceberg scan is on by default in 1.0. ## What changes are included in this PR? Included: - The `NamedReference` guard. `icebergExprToProto` declines any term that is not a `NamedReference`, and reads the column name from the term itself. - The doc comment on `icebergExprToProto` now says iceberg-rust filters rows by the pushed predicate, instead of calling it a pruning hint. The catch block's comment no longer calls the residual a hint either. - `CometIcebergResidualPushdownSuite`, unchanged from upstream, registered in the suite lists of `pr_build_linux.yml` and `pr_build_macos.yml`. Left out: - The half that fails the query when a residual cannot be converted (`serializeResidual`), which is what #5992 asks for. On `branch-1.0` a conversion failure still logs a warning and pushes nothing, which is safe for results: pushing less only loses pruning. Turning those failures into query failures is a behaviour change I'd rather not make in a patch release. - The unit tests #6154 added to `CometIcebergNativeScanSuite`. They test `serializeResidual`, and that suite does not exist on `branch-1.0`. - `IcebergReflection.getMethod`, which comes from #5222. The converter keeps `branch-1.0`'s plain reflection. The nested-field comment is rewrapped because the code under the new `if` moves one level in, and the line would otherwise break Scalastyle's 100-character limit. ## How are these changes tested? Run locally on `branch-1.0` with the default Spark 4.1 profile, Iceberg 1.11.0 and JDK 17: - `CometIcebergResidualPushdownSuite` passes. Its test runs five queries that combine `bucket`, `truncate` or `days` with `AND`, `OR` and `NOT`, compares each with Spark, and checks that each still runs on the native Iceberg scan. - The bug is present on `branch-1.0`, and the test catches it. With `CometIcebergNativeScan.scala` reverted and the test kept, it fails on its first query, `bucket(4, id) = 2 AND data > 'a'`: Spark returns 22 rows and Comet returns none. - The full `CometIcebergNativeSuite` passes, 97 tests. - Scalastyle, through `test-compile` on the default profile, Spotless, and scalafix in CHECK mode on Spark 3.5 pass. I ran them once with all six `branch-1.0` backports from this round applied together. `run-iceberg-tests` is copied from #6154, so CI runs the Iceberg Spark suites here too. ## Are there any user-facing changes? Iceberg queries that combine a system-function filter with other predicates now return all their rows. Residuals over plain columns are pushed down as before. There are no config or API changes. -- 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]
