The purpose of these knobs are for users to turn off rules that are taking too long / produces suboptimal plan. All rules are enabled by default.

I suppose these knobs are not just for debugging. It is expected that users may use this in their production workloads. Correct?

The naming convention here is consistent with the rule name shown on query console from EXPLAIN PLAN FOR, it's just plannerRule.skip.

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.

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 CORE_RULES the description is the same as class name), so we could "rename" rules just using withDescription() when creating them. This applies to both Calcite rules and our custom rules.
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

so we could "rename" rules just using withDescription() when creating them.

That may not be guaranteed to be the same across Calcite version upgrades. I suppose Calcite won't change class names since that would be a breaking change, but descriptions could be changed.

That may not be guaranteed to be the same across Calcite version upgrades.

I see. Our current explain plan uses RelOptRule.toString() as the display name, which returns the rule description. This is also subject to change if the default description is changed in Calcite.

So I'm proposing to explicitly assign a description with a defined constant when instantiating each rule. In this way the identifier of the rule would always be controlled by us (which will always be its description), renaming rules would be easy by just changing the constant, and we can upgrade / replace certain rules with Pinot version while keeping the same name. I think this is similar to the idea of defining a wrapper around rules, except we utilize the description field of RelOptRules, which is intended for this.

Assigning descriptions to rules should be a good practice as it enables us to distinguish instances of same rule class under different context, plus we would have a nice list of all rule name constants used in CommonConstants, which is easier to maintain.

For example, in PinotQueryRuleSets.BASIC_RULES, we instantiate rule like

JoinPushExpressionsRule.Config.DEFAULT
    .withDescription(PlannerRuleNames.JOIN_PUSH_EXPRESSIONS).toRule()

Now we can rename it easily or upgrade it to something like PinotJoinPushExpressionsRule with the same description. It also won't be effected by Calcite change. If we were to add another instance of JoinPushExpressionsRule somewhere else, we can provide a different description to distinguish. Also all used names would be shown under PlannerRuleNames.

Please let me know if this makes sense?

Codecov Report

Attention: Patch coverage is 96.93878% with 3 lines in your changes missing coverage. Please review.

Project coverage is 63.22%. Comparing base (1a476de) to head (0828211).
Report is 288 commits behind head on master.

@@             Coverage Diff              @@
##             master   #15999      +/-   ##
============================================
+ Coverage     62.90%   63.22%   +0.31%     
+ Complexity     1386     1352      -34     
============================================
  Files          2867     2951      +84     
  Lines        163354   169639    +6285     
  Branches      24952    25943     +991     
============================================
+ Hits         102755   107249    +4494     
- Misses        52847    54279    +1432     
- Partials       7752     8111     +359     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

IMHO, it would be a better idea to introduce a single skipPlannerRules=[PinotAggregateReduceFunctionsRule, ProjectJoinTransposeRule, ...]

changed option format to
SET skipPlannerRules='FilterProjectTransposeRule,PruneEmptySort';