brijrajk commented on PR #12151: URL: https://github.com/apache/gluten/pull/12151#issuecomment-5104651605
@zhztheplayer Good question, and the underlying insight is right: byte-format compatibility only depends on which expression class runs (`VeloxBloomFilterAggregate`/`VeloxBloomFilterMightContain` vs. the vanilla ones), not on whether the surrounding operator ended up native or JVM-fallback. That's exactly why the `injectFinal` re-application is safe to add on top of the existing rules. I built the literal version of the proposal to check it properly before replying: one rule, unconditionally rewriting every `bloom_filter_agg`/`might_contain` to Velox form (no literal/non-literal distinction, no `xxhash64` restriction), registered only at `injectFinal`. Three real regressions showed up: **1. `DataFrame.stat.bloomFilter()` breaks.** Its standalone `bloom_filter_agg` (not paired with any `might_contain`) gets swept up by an unconditional match and rewritten to Velox format, and Spark's own `BloomFilter.readFrom()` can't parse it: ``` java.io.IOException: Unexpected Bloom filter version number (16777472) ``` **2. Native offload is lost for runtime bloom filters.** Confirmed via plan diff: the producing aggregate ran as vanilla `ObjectHashAggregate` and the consuming filter as plain `Filter` wrapped in `RowToVeloxColumnar`, even though nothing structurally prevented native execution. Cause: `HeuristicTransform`'s offload validator runs before `injectFinal`, so it sees the still-vanilla expression and rejects native offload for that operator -- by the time `injectFinal` swaps in the Velox class, the physical operator type is already locked in. This is the same R2C/C2R regression that was the reason the physical `injectPreTransform` registration got added for runtime filters earlier in this PR's review. TPCH q19's plan-stability golden also diverged under this build, so it's not an isolated case. **3. SPARK-54336's NULL-on-empty-input semantics silently break.** ``` == Correct Answer == == Gluten Answer == [null] [false] ``` Not a crash -- a query that should return `null` returns `false` instead, because the literal-value pair no longer stays vanilla. So I don't think we can collapse to a single late rule: fixing correctness on reversion (what `injectFinal` is for) and getting native offload in the common case (which needs the rewrite to exist before `HeuristicTransform` validates) are two different requirements, and a single late hook can only satisfy one of them -- the offload decision is irreversible by the time `injectFinal` runs. Separately, unconditional matching (dropping the pairing/fingerprint check) isn't safe regardless of timing, since nothing then distinguishes a `bloom_filter_agg` feeding a `might_contain` from a standalone `stat.bloomFilter()` call. Happy to explore a narrower version, though -- e.g. dropping the literal-value special case in the logical rule so both literal and non-literal always rewrite to Velox, if we first close the NULL-on-empty-input gap in `VeloxBloomFilterMightContain`/`VeloxBloomFilterAggregate`. That would remove one axis of complexity without touching the injection points. Let me know if that's the kind of simplification you had in mind, or if I'm missing something in what you were picturing. -- 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]
