brijrajk commented on PR #12151: URL: https://github.com/apache/gluten/pull/12151#issuecomment-5197227280
@zhztheplayer You were right, and the logical rule is not needed. I have rewritten the PR to the one-rule `injectPre` design you asked for. Rebased onto current main. The whole PR is now: - `BloomFilterMightContainJointRewriteRule` stays exactly as it is on main: a physical rule, unconditional, guarded by `c.caller.isBloomFilterStatFunction()`. I reverted my conversion of it to a logical optimizer rule, and restored the `CallerInfo` flag I had removed. - Its registration moves from `injectPreTransform` to `injectPre`. That is the entire production change. - `RuntimeBloomFilterRewriteRule` is deleted. It is unnecessary: the unconditional rewrite already covers `InjectRuntimeFilter`'s expressions, and the runtime-filter reversion test passes without it. - The regression suite stays. Production diff is now 12 added / 5 removed lines in `VeloxRuleApi.scala`, versus 629/77 across 5 files before. I should have tested your proposal before arguing against it. What I had been measuring was my own conditional rule ported to a physical hook, which does need to reach into the subquery's plan and cannot, because `AdaptiveSparkPlanExec` is a `LeafExecNode` and traversal stops there. Your design never needs that reach-in: with an unconditional rewrite the producer is rewritten when Gluten's rules run on the subquery's own `AdaptiveSparkPlanExec`, so both sides agree by construction and nothing has to cross that boundary. That also removes the need for the SPARK-54336 literal special case, which only existed because I had moved the rule to the logical level. Measured on Spark 4.0 / Scala 2.13: | Configuration | Result | | --- | --- | | current main | bug reproduces: `SparkException` in `ScalarSubquery.updateResult`, `Unsupported BloomFilter version: 0` | | main + registration at `injectPre` | fallback suite 8/8, aggregate + runtime filter + stat + subquery 270/270, plan stability 322/322, no crashes | Why the hook matters: `injectPreTransform` runs after `HeuristicApplier` captures the plan `ExpandFallbackPolicy` reverts to, so a whole-stage revert strips the rewrite from one stage while another keeps it. `injectPre` runs before that capture, so the rewrite is part of the revert target. The byte-format mismatch is concrete: Velox writes the version as a single `int8` of 1, while Spark's `BloomFilterImpl.writeTo` writes a 4-byte big-endian int, so vanilla bytes start with `0x00` and the native reader reports version 0. Two notes on the tests: - The two whole-stage-fallback tests previously asserted `optimizedPlan` contained `velox_might_contain`. That only holds for a logical rule, so they now assert on `executedPlan`, which is what actually determines whether the rewrite survived the fallback. - The SPARK-54336 test no longer asserts that both sides stay vanilla. It asserts the invariant that matters: never `velox_might_contain` without a matching `velox_bloom_filter_agg`. This also relies on `VeloxBloomFilterAggregate`'s JVM-side buffer sizing agreeing with the native aggregate's (GLUTEN-12613, merged), otherwise a reverted stage merging with a native one corrupts the filter silently instead of failing loudly. -- 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]
