brijrajk commented on PR #12151: URL: https://github.com/apache/gluten/pull/12151#issuecomment-4938649075
@zhztheplayer @philo-he you were both right to push on the approved-plan changes, and I owe you a correction before anything else. **Retraction.** My June 30 comment claimed the q59 fallback was intentional and that a "BloomFilterRuntimeFilterRewriteRule" patched the subquery aggregate. That analysis was wrong: no such mechanism existed on the branch, and the regenerated goldens themselves showed vanilla `might_contain` / `bloom_filter_agg` (not the Velox variants) with a JVM `Filter` and `ObjectHashAggregate`. The fallbacks were a real regression, and regenerating the goldens masked it instead of fixing it. I apologize for the confusion this caused. **Root cause (verified against Spark sources).** `injectOptimizerRule` registers into `extendedOperatorOptimizationRules`, which run inside the Operator Optimization batch of the optimizer. `InjectRuntimeFilter` runs in a later batch of `SparkOptimizer` (after `super.defaultBatches`). So after the rule was moved to the logical level, runtime-filter bloom expressions did not exist yet when it fired. They were never rewritten to the Velox variants, had no Substrait mapping, and both the consuming `FilterExec` and the producing aggregate fell back to the JVM with R2C/C2R transitions. The earlier `!v.isInstanceOf[Literal]` guard change could not have fixed this, for the same ordering reason. **Fix (pushed).** The rewrite is now split by where the expressions become visible: - User-facing `might_contain(<scalar subquery>, <non-literal>)` pairs: still handled by the logical `BloomFilterMightContainJointRewriteRule` (your suggestion, @zhztheplayer; it remains the actual GLUTEN-12013 fix, since it bakes the substitution into the `originalPlan` snapshot that `ExpandFallbackPolicy` uses for whole-stage AQE fallback). - Runtime-filter pairs: handled by a new physical pre-transform rule, `RuntimeBloomFilterRewriteRule`, restricted to `InjectRuntimeFilter`'s exact shape: both producer and consumer always wrap the key in `xxhash64(...)`. Each side is identifiable on its own, so the rewrite stays consistent even when AQE compiles the subquery separately. This restores the pre-PR native `FilterExecTransformer` behavior. - `DataFrame.stat.bloomFilter()` builds `bloom_filter_agg` on the raw column (no `xxhash64` wrapper), so it is structurally never matched, and the `CallerInfo` stack-walk stays removed as @rdtr requested. - Literal-valued pairs (SPARK-54336) contain no `xxhash64` and stay fully vanilla on both sides. **Goldens.** With runtime filters native again, no TPC-DS plans change at all. The golden-regeneration commit is dropped and the PR is back to a single commit, rebased onto current main. That directly answers the "why do we need to update them" question: we should not have. **Tests (all green locally, Spark 4.0 Velox backend).** - `GlutenBloomFilterFallbackSuite`: 7/7, including a new regression test asserting a runtime bloom filter keeps a native `FilterExecTransformer` evaluating `velox_might_contain` with a `VeloxBloomFilterAggregate` producer, so this fallback cannot silently return. - `GlutenBloomFilterAggregateQuerySuite`: 14/14, including SPARK-54336. - All 7 TPC-DS/TPC-H plan-stability suites against the unchanged main goldens: 322/322. -- 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]
