brijrajk commented on PR #12151: URL: https://github.com/apache/gluten/pull/12151#issuecomment-5142658656
@zhztheplayer Took your `injectPre` idea and actually built it, to check it properly before replying. **What I implemented:** - A new `injectPre` hook on `GlutenInjector.LegacyInjector`, backed by an empty-by-default builder list (mirrors the existing hooks -- additive only, no changes needed anywhere that doesn't opt in). - `HeuristicApplier` runs a new `"pre"` phase before `"transform"`, and `fallbackPolicies` now closes over the post-`"pre"` plan instead of the raw input -- so a whole-stage revert can no longer strip away anything registered there. - Switched `RuntimeBloomFilterRewriteRule` to register once, at `injectPre`, dropping both current registrations (`injectPreTransform` + `injectFinal`). **Result on Velox: it works, cleanly.** Full suite passes with the collapsed single registration: - `GlutenBloomFilterFallbackSuite`: 8/8, including both whole-stage-reversion tests and the native-offload test - `GlutenBloomFilterAggregateQuerySuite`: 28/30 (2 known pre-existing/unrelated failures) - `GlutenInjectRuntimeFilterSuite`: 13/13 - Full TPC-DS/TPC-H plan-stability: 322/322 So the mechanism is sound, and it does genuinely simplify things for the runtime-filter rule specifically. **Checked ClickHouse too**, since this touches shared `gluten-core`: `CHRuleApi.scala` needs zero changes and builds, links, and packages successfully against the modified core. That's expected since it never calls `injectPre` (the new builder list stays empty for CH), but wanted it verified against a real build rather than just argued. **Why I'm not just pushing this into #12151**: it's a `gluten-core`/`HeuristicApplier` change, shared across every backend, not scoped to bloom filters at all. Given how this PR has already gone once around on scope (the capacity fix got split into #12614), I'd rather do the same here: propose `injectPre` as its own follow-up PR once we're aligned on the API shape, and keep #12151 as the three-rule version it already is, which is correct and fully tested. Happy to open that PR, or if you'd rather drive the core API design yourself given you proposed it, that works too. Let me know which you'd prefer. -- 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]
