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: [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]
