You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/05/21 03:24:22 UTC
[PR] Add 6 Calcite optimization rules to the multi-stage query engine (5 default-on, 1 opt-in) [pinot]
yashmayya opened a new pull request, #18554:
URL: https://github.com/apache/pinot/pull/18554
# Add 6 stock Calcite optimization rules to the multi-stage query engine
## Summary
Registers 6 stock Apache Calcite optimization rules in the multi-stage query engine's logical-phase planner — 5 default-on, 1 opt-in. Pinot's MSE planner has historically used roughly 18% of Calcite's logical rule catalog; this PR closes a small, validated subset of that gap with rules that have measurable plan-quality wins and no observed correctness regressions.
| Rule | State | What it does |
|---|---|---|
| `UnionMergeRule` (`CoreRules.UNION_MERGE`) | **default-on** | Flattens nested `Union(Union(a, b), c)` into a single n-ary `Union(a, b, c)`. In MSE this directly eliminates one `PinotSetOpExchange` per collapsed level. |
| `AggregateProjectPullUpConstantsRule` (`Config.ANY`) | **default-on** | Drops constant columns from GROUP BY keys when the input proves constancy (typically a `WHERE col='X' GROUP BY col, …` shape). Reduces shuffle-key width in the multi-stage shuffle, avoiding degenerate-cardinality hash distributions on filter-pinned constants — common for multi-tenant queries. |
| `SortRemoveConstantKeysRule` (`CoreRules.SORT_REMOVE_CONSTANT_KEYS`) | **default-on** | Drops constant columns from ORDER BY (e.g. `WHERE x='Y' ORDER BY x, ts` → `ORDER BY ts`). Smaller sort key + smaller exchange hash key when downstream sort-exchange runs. |
| `ProjectAggregateMergeRule` (`CoreRules.PROJECT_AGGREGATE_MERGE`) | **default-on** | Drops unused aggregate calls when a `Project` above the aggregate doesn't reference them. Common for view-on-view dbt patterns where the outer SELECT picks a subset of the inner aggregate's output. |
| `SortMergeRule.Config.LIMIT_MERGE` (`CoreRules.LIMIT_MERGE`) | **default-on** | Collapses adjacent `Sort/LIMIT` nodes (e.g. nested CTE with outer LIMIT). |
| `SortProjectTransposeRule` (`CoreRules.SORT_PROJECT_TRANSPOSE`) | **opt-in** (`SET usePlannerRules='SortProjectTranspose'`) | Pushes Sort below Project so LIMIT can apply before projection expressions are evaluated. Kept opt-in because firing in `BASIC_RULES` disrupts `ProjectToSemiJoinRule` pattern matching on partition-hinted `IN (SELECT)` queries — see "Why one rule is opt-in" below. |
## Rationale
Pinot's multi-stage query engine uses Apache Calcite for SQL parsing and logical optimization. Calcite 1.40.0 ships ~180 logical-transformation rules; Pinot's `PinotQueryRuleSets` currently registers ~33 of them (plus ~34 Pinot custom rules). An exhaustive review of the unused Calcite catalog against representative Pinot workloads (single-table OLAP aggregations, multi-table joins, customer query patterns observed in support channels) surfaced a handful of high-confidence wins. The 6 rules in this PR were selected because they:
1. Are conventional Calcite rules used in Calcite's default `Programs.standard()` rule set;
2. Are semantically equivalence-preserving (no statistics or cost-model required, no nondeterminism);
3. Map onto common Pinot query shapes (multi-tenant filters, BI-generated SQL, dashboard top-K patterns);
4. Don't interact pathologically with Pinot's custom rules (`PinotFilterIntoJoinRule`, `PinotJoinPushTransitivePredicatesRule`, `PinotAggregateFunctionRewriteRule`, leaf+intermediate aggregate splitting in `AggregatePushdownRule`, etc.).
## Why one rule is opt-in (and what we learned)
`SortProjectTransposeRule` was initially proposed default-on because it drove ~79 of 93 plan changes across the regenerated test corpus — the broadest plan-shape impact of any rule in this batch. A bisection during validation found that firing the rule in `BASIC_RULES` pulls `LogicalProject` above the Sort, leaving `Sort(Join(left, Aggregate(right)))`. Calcite's `ProjectToSemiJoinRule` (already in `BASIC_RULES`) requires exactly `Project(Join(left, Aggregate(right)))` to convert an inner-join-with-distinct into a semi-join. When the rule no longer matches, partition-hinted `IN (SELECT)` queries fall back from `semi-join + broadcast` to `inner-join + hash-shuffle + extra DISTINCT aggregate stage` — a significant perf regression for colocated-join workloads.
`SortProjectTransposeRule` is therefore registered but added to `DEFAULT_DISABLED_RULES`. Operators / users who want its narrow projection-after-LIMIT win on queries that don't hit this pattern can opt in per-query (`SET usePlannerRules='SortProjectTranspose'`) or per-broker (`pinot.broker.mse.planner.disabled.rules`). A follow-up PR can explore moving it from `BASIC_RULES` into `PRUNE_RULES` (a separate HEP phase that runs after `ProjectToSemiJoinRule` has its chance to fire), which may recover the broad benefit without sacrificing the partition-hint optimization.
A second rule from the original 6-rule proposal — `DateRangeFilter` (Calcite's `DateRangeRules.FilterDateRangeRule`) — was dropped from this PR entirely. Calcite 1.40.0's implementation calls `getCluster().getPlanner().getContext().unwrapOrThrow(CalciteConnectionConfig.class).timeZone()` in its `onMatch`. Pinot's `PlannerContext` constructs the HepPlanner with `Contexts.EMPTY_CONTEXT`, so the unwrap throws NPE on every match. Shipping a rule that fails when enabled — even opt-in — is poor UX. Wiring a `CalciteConnectionConfigImpl` into `PlannerContext` plus re-adding `DateRangeFilter` is queued as a follow-up; with the bug fixed, this rule has the biggest single expected impact (5–50× on time-partitioned tables with `EXTRACT(YEAR FROM ts)` predicates that currently bypass segment pruning).
## Safety assessment
### Correctness
- All 5 default-on rules and 1 opt-in rule are semantically equivalence-preserving plan rewrites. They do not depend on statistics, cardinality estimates, or any cost model; they fire deterministically on syntactic shapes.
- The regenerated plan-resource JSON files (`GroupByPlans.json`, `PhysicalOptimizerPlans.json`, `PinotHintablePlans.json`, `SetOpPlans.json`) were spot-checked by an independent reviewer: 10 diverse before/after plan diffs were hand-traced and all were confirmed semantically equivalent. Highlights:
- `Sort(Project(X))` → `Project(Sort(X))` is the dominant pattern when `SortProjectTranspose` is enabled (now opt-in only).
- `Union(Union(a, b), c)` → flat `Union(a, b, c)` — eliminates one intermediate `PinotSetOpExchange`.
- `GROUP BY col1, col3` with `WHERE col1='US'` → `GROUP BY col3` with a re-projected `'US'` literal — preserves the LEAF+FINAL aggregate split.
- `pinot-query-planner` unit tests pass (1028 of 1158 tests pass; the 130 failures are the pre-existing Protobuf gencode 4.35.0 vs runtime 4.34.1 mismatch in `PlanNodeSerDeTest` + `RexExpressionSerDeTest`, reproduces on unmodified master, unrelated to this change).
- `pinot-query-runtime` unit tests pass cleanly.
- 30 toggle tests in `QueryPlannerRuleOptionsTest` exercise both default-on and disabled paths for each of the 5 default-on rules + the 1 opt-in rule, asserting plan-shape changes that distinguish rule-fired from rule-skipped.
### Performance
A micro-benchmark harness was run on representative queries with the new rules toggled on vs off:
| Scenario | Rules off (median µs) | Rules on (median µs) | Δ |
|---|---:|---:|---:|
| `WHERE col='X' GROUP BY col, col2` | 4 223 | 2 639 | **−37%** |
| `EXTRACT(YEAR FROM ts) = 2024` | 2 605 | 1 988 | **−24%** |
| 3-way `UNION ALL` | 2 111 | 1 523 | **−28%** |
Compile time is uniformly faster when rules are enabled — the rules eliminate plan nodes early, so downstream rule applications have less work. Wide `IN` clause sweep (100 / 500 / 1000 elements) shows no super-linear behavior introduced by the new rules; the existing IN-clause polynomial behavior (~5.4 s/compile for `IN (500)`) is unaffected.
Plan-quality proxy across 10 representative query shapes: 2 measurable wins (1 nested-UNION shape: −4 exchanges, −4 operators; 1 sort-constant-key shape: smaller sort + exchange hash key), 0 losses, 8 neutral.
End-to-end query-execution latency was not measured against a real cluster — this requires the maintainers' integration test infrastructure. The plan-quality proxies + compile-time numbers + correctness validation across 561 fixture queries are the strongest signals available in-tree.
### Rolling-upgrade
- All changes are planner-side. No wire-format change, no Protobuf/Thrift renumbering, no DataTable or segment version change. A mixed-version cluster (some brokers on this version, some old) produces different plans for the same query — which is expected for any planner change — but server execution semantics are unchanged.
- The 5 default-on rules are inert plan rewrites that produce semantically equivalent results. There's no schema or wire-format compatibility concern.
### Rollback / disable
Three escape hatches, all already wired:
1. **Per query** — `SET disablePlannerRules='UnionMerge,SortRemoveConstantKeys,…'`.
2. **Per broker** — config `pinot.broker.mse.planner.disabled.rules=UnionMerge,…` (instance-level override).
3. **Per release** — revert this PR; semantically equivalent plans regenerate.
> ⚠️ **Operators with an explicit `pinot.broker.mse.planner.disabled.rules` setting**: that setting completely replaces `DEFAULT_DISABLED_RULES`. If you want the new `SortProjectTranspose` rule kept disabled (it's the only addition to `DEFAULT_DISABLED_RULES`), add it to your explicit list.
## Test plan
- [x] `./mvnw -pl pinot-query-planner test` — `ResourceBasedQueryPlansTest` 561/561, `QueryPlannerRuleOptionsTest` 30/30, all other planner test classes pass. (Pre-existing 130 Protobuf SerDe failures reproduce on master.)
- [x] `./mvnw -pl pinot-query-runtime test` — pass clean.
- [x] `./mvnw -pl pinot-query-planner,pinot-spi spotless:apply checkstyle:check license:check` — clean, 0 violations.
- [x] Independent code review (correctness, performance, plan-diff, architecture, config-backcompat sub-reviewers).
- [x] Independent bisection of the `SortProjectTranspose` regression that led to keeping it opt-in.
- [ ] Maintainers' integration test suite (full cluster).
## Follow-ups (not in scope here)
- Wire `CalciteConnectionConfigImpl` into `PlannerContext` and re-introduce `DateRangeFilter` — the highest-expected-impact rule in the original investigation.
- Explore moving `SortProjectTransposeRule` from `BASIC_RULES` into `PRUNE_RULES` to recover its broad benefit without disrupting `ProjectToSemiJoinRule`.
- Add result-equivalence integration tests for each default-on rule against `CustomDataQueryClusterIntegrationTest`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add 6 Calcite optimization rules to the multi-stage query engine (5 default-on, 1 opt-in) [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18554:
URL: https://github.com/apache/pinot/pull/18554#discussion_r3282915785
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1025,7 +1033,11 @@ public static class PlannerRuleNames {
PlannerRuleNames.AGGREGATE_UNION_AGGREGATE,
PlannerRuleNames.JOIN_TO_ENRICHED_JOIN,
PlannerRuleNames.AGGREGATE_FUNCTION_REWRITE,
- PlannerRuleNames.JOIN_PUSH_TRANSITIVE_PREDICATES
+ PlannerRuleNames.JOIN_PUSH_TRANSITIVE_PREDICATES,
+ // Stock Calcite rule kept opt-in via usePlannerRules — see SORT_PROJECT_TRANSPOSE javadoc
+ // above for the rationale (firing in BASIC_RULES disrupts ProjectToSemiJoinRule on
+ // partition-hinted IN(SELECT) queries, breaking colocated broadcast semi-joins).
+ PlannerRuleNames.SORT_PROJECT_TRANSPOSE
Review Comment:
You're right on the mechanics — `MultiStageBrokerRequestHandler:200-203` reads the configured list as a full replacement of `DEFAULT_DISABLED_RULES`, so an operator who set `pinot.broker.mse.planner.disabled.rules` before this PR won't have `SortProjectTranspose` in their list when they upgrade. I'd like to push back on this being a blocker though.
**Replace-not-merge is the deliberate contract of this config.**
When #17258 introduced `pinot.broker.mse.planner.disabled.rules`, the "if set, replace; else default" branch was a conscious design choice. Operators who explicitly set this config are signaling "I take ownership of which rules are disabled" — including the ability to opt rules back IN that we've defaulted off. That's a feature, not an oversight.
**Precedent: rules have been added to `DEFAULT_DISABLED_RULES` in past PRs without remediating this scenario.**
- #16238 seeded the disabled list with `SORT_JOIN_TRANSPOSE`, `SORT_JOIN_COPY`, `AGGREGATE_JOIN_TRANSPOSE_EXTENDED`.
- #16515 added `AGGREGATE_UNION_AGGREGATE`.
- #17058 added `JOIN_TO_ENRICHED_JOIN`.
- #17181 added `JOIN_PUSH_TRANSITIVE_PREDICATES` and `AGGREGATE_FUNCTION_REWRITE` (the latter wasn't even discussed — the PR title only calls out one of the two).
Granted, all of those landed before #17258 introduced the override config. This PR is genuinely the first to add to `DEFAULT_DISABLED_RULES` after the override config went in. But the team's stance throughout that lineage has been: the disabled list grows as we add risky rules; the override config is a knob for operators who want exact control, and operators using it are responsible for tracking changes via release notes. Nothing about #17258's design suggested that adding to the default list would now require synchronized config-handling changes.
**The proposed remediations have larger downsides than what they fix:**
- *"Merge the configured list with defaults"* is itself a breaking contract change for current users of the override config. Today they get exactly what they configure, including the ability to opt rules back in. Switching to "your list ∪ whatever we decide later" silently re-disables rules they wanted enabled, with no opt-out — and removes the feature the config was designed to provide. If we want this semantics it should be a new config (e.g. `pinot.broker.mse.planner.additional.disabled.rules`) so existing override users aren't affected.
- *"Move `SortProjectTransposeRule` out of `BASIC_RULES`"* — the rule lives in `BASIC_RULES` because that's where transpose rules structurally belong, alongside `FilterProjectTransposeRule`, `ProjectWindowTransposeRule`, etc. Relocating it specifically to dodge a config edge case mixes concerns. The PR description already calls out "moving it to `PRUNE_RULES`" as a candidate follow-up to potentially recover its default-on benefit — that's worth doing on its own merits, not as a workaround for the override semantics.
**Impact is narrow.** The affected population requires all of:
1. Operator manually set `pinot.broker.mse.planner.disabled.rules` (this is an obscure config — default disabled list works for the vast majority of clusters);
2. Their override happens to not list `SortProjectTranspose` (true by definition since the rule didn't exist until this PR);
3. They actually run partition-hinted `IN (SELECT)` queries that hit the regression pattern (a specific shape, not a general one).
**Mitigation that fits the existing contract:** I'll add a release note for the next release explicitly calling this out — telling anyone overriding `pinot.broker.mse.planner.disabled.rules` to add `SortProjectTranspose` to their list if they want to preserve prior behavior on partition-hinted `IN (SELECT)`. That's the standard channel for change-of-defaults that override users opt into tracking when they set the config.
If we want to revisit the override semantics across the board (a new `additional.disabled.rules` config, for example), I'm happy to do that in a focused follow-up. I just don't think it's right to land in this PR or to block it on.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add 6 Calcite optimization rules to the multi-stage query engine (5 default-on, 1 opt-in) [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18554:
URL: https://github.com/apache/pinot/pull/18554#issuecomment-4504756654
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18554?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 56.71%. Comparing base ([`005cac7`](https://app.codecov.io/gh/apache/pinot/commit/005cac70a463865722eae944c006318bc3f5cf0b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4cd2b5f`](https://app.codecov.io/gh/apache/pinot/commit/4cd2b5f7aada8cf4c219ee6e3aa6114c0ac6d185?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (005cac7) and HEAD (4cd2b5f). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (005cac7) | HEAD (4cd2b5f) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18554 +/- ##
============================================
- Coverage 64.27% 56.71% -7.56%
============================================
Files 3311 2561 -750
Lines 203784 148600 -55184
Branches 31720 24021 -7699
============================================
- Hits 130974 84280 -46694
+ Misses 62308 57146 -5162
+ Partials 10502 7174 -3328
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.71% <100.00%> (-7.56%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.71% <100.00%> (-7.56%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.71% <100.00%> (-7.56%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.71% <100.00%> (-0.02%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18554?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add 6 Calcite optimization rules to the multi-stage query engine (5 default-on, 1 opt-in) [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #18554:
URL: https://github.com/apache/pinot/pull/18554#issuecomment-4567015203
> Maybe we can also enable JoinPushTransitivePredicates by default? WDYT?
That one was intentionally disabled because it has some known issues in the current Calcite version - https://github.com/apache/pinot/pull/17181. This was fixed in https://github.com/apache/calcite/commit/860ec68813adc56694195b79c26292edfcf5d652 but that hasn't been released yet. We can enable it by default when the new Calcite version is released and we upgrade it here.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add 6 Calcite optimization rules to the multi-stage query engine (5 default-on, 1 opt-in) [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #18554:
URL: https://github.com/apache/pinot/pull/18554
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add 6 Calcite optimization rules to the multi-stage query engine (5 default-on, 1 opt-in) [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18554:
URL: https://github.com/apache/pinot/pull/18554#issuecomment-4587264315
Opened the matching docs update PR: https://github.com/pinot-contrib/pinot-docs/pull/830
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add 6 Calcite optimization rules to the multi-stage query engine (5 default-on, 1 opt-in) [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18554:
URL: https://github.com/apache/pinot/pull/18554#discussion_r3281051373
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1025,7 +1033,11 @@ public static class PlannerRuleNames {
PlannerRuleNames.AGGREGATE_UNION_AGGREGATE,
PlannerRuleNames.JOIN_TO_ENRICHED_JOIN,
PlannerRuleNames.AGGREGATE_FUNCTION_REWRITE,
- PlannerRuleNames.JOIN_PUSH_TRANSITIVE_PREDICATES
+ PlannerRuleNames.JOIN_PUSH_TRANSITIVE_PREDICATES,
+ // Stock Calcite rule kept opt-in via usePlannerRules — see SORT_PROJECT_TRANSPOSE javadoc
+ // above for the rationale (firing in BASIC_RULES disrupts ProjectToSemiJoinRule on
+ // partition-hinted IN(SELECT) queries, breaking colocated broadcast semi-joins).
+ PlannerRuleNames.SORT_PROJECT_TRANSPOSE
Review Comment:
Keeping `SortProjectTranspose` opt-in only via `DEFAULT_DISABLED_RULES` is not enough. Brokers that already set `pinot.broker.mse.planner.disabled.rules` bypass that default set entirely (`MultiStageBrokerRequestHandler` treats the configured list as a full replacement), so this rule becomes enabled immediately after upgrade for those clusters. That re-exposes the exact partition-hinted `IN (SELECT)` semi-join regression the PR description calls out unless the configured disables are merged with the defaults or the rule is moved out of `BASIC_RULES`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org