songwdfu commented on PR #15999: URL: https://github.com/apache/pinot/pull/15999#issuecomment-2945259928
> I suppose these knobs are not just for debugging. It is expected that users may use this in their production workloads. Correct? Yes. One intended use case is that, user sees the rule timing by running EXPLAIN PLAN FOR, sees specific rule taking too long, then simply copy the rule name and prepend it with plannerRule.skip to disable it. The current naming convention accounts for this scenario as well. For users who knows which plan resulted in the bad plan, they could go ahead and disable it as well. > I think this makes sense to some extent for rules in Calcite. But for rules implemented by us, we should instead use shorter names. We could create a new abstract class that overrides `RelOptRule` and adds an abstract method to define a name to be used for this purpose. Certainly, there are rules whose name is too long / doesn't make sense. I found out our EXPLAIN PLAN FOR query timing simply uses rule's description (just that for Calcite rules the description is the same as class name), so we could "rename" rules just using withDescription() when creating them. This comes with three benefits: 1. it preserves naming consistency between EXPLAIN PLAN FOR and the knob, both are just the description of the rule. 2. we could now theoretically seperate same rules in different phases, e.g. `FilterAggregateTransposeRule` in `BASIC_RULES` vs `FILTER_PUSHDOWN_RULES`, if it's needed. 3. no special cases are needed now, checking logic in QueryEnvironment is simplified Will update this now if it makes sense -- 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]
