Jackie-Jiang commented on code in PR #15999:
URL: https://github.com/apache/pinot/pull/15999#discussion_r2148853064
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -664,6 +664,55 @@ public static class QueryOptionValue {
}
}
+ /**
+ * Calcite and Pinot rule names / descriptions
+ * used for enable and disabling of rules, this will be iterated through
in PlannerContext
+ * to check if rule is disabled.
+ */
+ public static class PlannerRuleNames {
+ public static final String PINOT_FILTER_INTO_JOIN =
"PinotFilterIntoJoinRule";
Review Comment:
Consider not adding `Pinot` in both variable name and value. We want to pick
a descriptive name for usability
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -664,6 +664,55 @@ public static class QueryOptionValue {
}
}
+ /**
+ * Calcite and Pinot rule names / descriptions
+ * used for enable and disabling of rules, this will be iterated through
in PlannerContext
+ * to check if rule is disabled.
+ */
+ public static class PlannerRuleNames {
+ public static final String PINOT_FILTER_INTO_JOIN =
"PinotFilterIntoJoinRule";
+ public static final String FILTER_AGGREGATE_TRANSPOSE =
"FilterAggregateTransposeRule";
+ public static final String FILTER_SET_OP_TRANSPOSE =
"FilterSetOpTransposeRule";
+ public static final String PINOT_PROJECT_JOIN_TRANSPOSE =
"ProjectJoinTransposeRule";
+ public static final String PROJECT_SET_OP_TRANSPOSE =
"ProjectSetOpTransposeRule";
+ public static final String FILTER_PROJECT_TRANSPOSE =
"FilterProjectTransposeRule";
+ public static final String PINOT_JOIN_CONDITION_PUSH =
"JoinConditionPushRule";
+ public static final String PINOT_JOIN_PUSH_TRANSITIVE_PREDICATES =
"JoinPushTransitivePredicates";
+ public static final String PROJECT_REMOVE = "ProjectRemoveRule";
+ public static final String PROJECT_TO_LOGICAL_PROJECT_AND_WINDOW =
"ProjectToLogicalProjectAndWindowRule";
+ public static final String PROJECT_WINDOW_TRANSPOSE =
"ProjectWindowTransposeRule";
+ public static final String PINOT_EVALUATE_LITERAL_PROJECT =
"EvaluateProjectLiteralRule";
+ public static final String PINOT_EVALUATE_LITERAL_FILTER =
"EvaluateFilterLiteralRule";
+ public static final String JOIN_PUSH_EXPRESSIONS =
"JoinPushExpressionsRule";
+ public static final String PROJECT_TO_SEMI_JOIN =
"ProjectToSemiJoinRule";
+ public static final String PINOT_SEMIN_JOIN_DISTINCT_PROJECT_RULE =
"SeminJoinDistinctProjectRule";
+ public static final String UNION_TO_DISTINCT = "UnionToDistinctRule";
+ public static final String AGGREGATE_REMOVE = "AggregateRemoveRule";
+ public static final String AGGREGATE_JOIN_TRANSPOSE =
"AggregateJoinTransposeRule";
+ public static final String AGGREGATE_UNION_AGGREGATE =
"AggregateUnionAggregateRule";
+ public static final String PINOT_AGGREGATE_REDUCE_FUNCTIONS =
"AggregateReduceFunctionsRule";
+ public static final String AGGREGATE_CASE_TO_FILTER =
"AggregateCaseToFilterRule";
+ public static final String PROJECT_FILTER_TRANSPOSE =
"ProjectFilterTransposeRule";
+ public static final String PROJECT_MERGE = "ProjectMergeRule";
+ public static final String AGGREGATE_PROJECT_MERGE =
"AggregateProjectMergeRule";
+ public static final String FILTER_MERGE = "FilterMergeRule";
+ public static final String SORT_REMOVE = "SortRemoveRule";
+ public static final String AGGREGATE_JOIN_TRANSPOSE_EXTENDED =
"AggregateJoinTransposeRuleExtended";
+ public static final String PRUNE_EMPTY_AGGREGATE = "PruneEmptyAggregate";
+ public static final String PRUNE_EMPTY_FILTER = "PruneEmptyFilter";
+ public static final String PRUNE_EMPTY_PROJECT = "PruneEmptyProject";
+ public static final String PRUNE_EMPTY_SORT = "PruneEmptySort";
+ public static final String PRUNE_EMPTY_UNION = "PruneEmptyUnion";
+ public static final String PRUNE_EMPTY_CORRELATE_LEFT =
"PruneEmptyCorrelateLeft";
+ public static final String PRUNE_EMPTY_CORRELATE_RIGHT =
"PruneEmptyCorrelateRight";
+ public static final String PRUNE_EMPTY_JOIN_LEFT = "PruneEmptyJoinLeft";
+ public static final String PRUNE_EMPTY_JOIN_RIGHT =
"PruneEmptyJoinRight";
+ }
+ public static final String PLANNER_RULE_SKIP = "plannerRule_skip";
+ public static String skipRule(String ruleString) {
+ return PLANNER_RULE_SKIP + ruleString;
+ }
Review Comment:
(minor) Move them into `PlannerRuleNames` class
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java:
##########
@@ -3502,12 +3502,15 @@ public void testExplainPlanQueryV2()
+ " LogicalFilter\\(condition=\\[<\\(.*, 0\\)]\\)\n"
+ " PinotLogicalTableScan\\(table=\\[\\[default, mytable]]\\)\n"
).matcher(response2Json.get("rows").get(0).get(1).asText()).find());
+ // TODO: investigate why changing description will result in changing of
order here
Review Comment:
Have you figured out why? If the same rule is applied multiple times, will
the time also be summed up?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -146,7 +148,6 @@ public QueryEnvironment(Config config) {
.defaultSchema(rootSchema.plus()).sqlToRelConverterConfig(PinotRuleUtils.PINOT_SQL_TO_REL_CONFIG).build();
_catalogReader = new PinotCatalogReader(
rootSchema, List.of(database), _typeFactory, CONNECTION_CONFIG,
config.isCaseSensitive());
- _optProgram = getOptProgram();
Review Comment:
We still want to keep this default `_optProgram`, so that for majority of
the cases (no rule override) we don't need to construct a new one
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -468,36 +472,111 @@ private DispatchableSubPlan
toDispatchableSubPlan(RelRoot relRoot, PlannerContex
// utils
// --------------------------------------------------------------------------
- private static HepProgram getOptProgram() {
+ /**
+ * Creates and returns a HepProgram that performs mostly logical
transformations.
+ * It performs several phases of rule application over the parsed
decorrelated trimmed plan:
+ * - In the first phase, it prunes the applies BASIC_RULES that are almost
always helpful to simplify logical plan
+ * - In the second phase, it performs predicate pushdown -> projection
pushdown -> predicate pushdown.
+ * - In the third phase, the logical plan is prune with PRUNE_RULES.
+ *
+ * @param options query options
+ * @return HepProgram that performs logical transformations
+ */
+ private static HepProgram getOptProgram(@Nullable Map<String, String>
options) {
HepProgramBuilder hepProgramBuilder = new HepProgramBuilder();
// Set the match order as DEPTH_FIRST. The default is arbitrary which
works the same as DEPTH_FIRST, but it's
// best to be explicit.
hepProgramBuilder.addMatchOrder(HepMatchOrder.DEPTH_FIRST);
// ----
+ // Rules are disabled if its corresponding value is set to false in
ruleFlags
+ // construct filtered BASIC_RULES, FILTER_PUSHDOWN_RULES,
PROJECT_PUSHDOWN_RULES, PRUNE_RULES
+ List<RelOptRule> basicRules;
+ List<RelOptRule> filterPushdownRules;
+ List<RelOptRule> projectPushdownRules;
+ List<RelOptRule> pruneRules;
+ if (options == null || noRulesSkipped(options)) {
+ basicRules = PinotQueryRuleSets.BASIC_RULES;
+ filterPushdownRules = PinotQueryRuleSets.FILTER_PUSHDOWN_RULES;
+ projectPushdownRules = PinotQueryRuleSets.PROJECT_PUSHDOWN_RULES;
+ pruneRules = PinotQueryRuleSets.PRUNE_RULES;
+ } else {
+ basicRules = filterRuleList(PinotQueryRuleSets.BASIC_RULES, options);
+ filterPushdownRules =
filterRuleList(PinotQueryRuleSets.FILTER_PUSHDOWN_RULES, options);
+ projectPushdownRules =
filterRuleList(PinotQueryRuleSets.PROJECT_PUSHDOWN_RULES, options);
+ pruneRules = filterRuleList(PinotQueryRuleSets.PRUNE_RULES, options);
+ }
+
+
// Run the Calcite CORE rules using 1 HepInstruction per rule. We use 1
HepInstruction per rule for simplicity:
// the rules used here can rest assured that they are the only ones
evaluated in a dedicated graph-traversal.
- for (RelOptRule relOptRule : PinotQueryRuleSets.BASIC_RULES) {
- hepProgramBuilder.addRuleInstance(relOptRule);
+ for (RelOptRule relOptRule : basicRules) {
+ hepProgramBuilder.addRuleInstance(relOptRule);
Review Comment:
(format) reformat
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -468,36 +472,111 @@ private DispatchableSubPlan
toDispatchableSubPlan(RelRoot relRoot, PlannerContex
// utils
// --------------------------------------------------------------------------
- private static HepProgram getOptProgram() {
+ /**
+ * Creates and returns a HepProgram that performs mostly logical
transformations.
+ * It performs several phases of rule application over the parsed
decorrelated trimmed plan:
+ * - In the first phase, it prunes the applies BASIC_RULES that are almost
always helpful to simplify logical plan
+ * - In the second phase, it performs predicate pushdown -> projection
pushdown -> predicate pushdown.
+ * - In the third phase, the logical plan is prune with PRUNE_RULES.
+ *
+ * @param options query options
+ * @return HepProgram that performs logical transformations
+ */
+ private static HepProgram getOptProgram(@Nullable Map<String, String>
options) {
HepProgramBuilder hepProgramBuilder = new HepProgramBuilder();
// Set the match order as DEPTH_FIRST. The default is arbitrary which
works the same as DEPTH_FIRST, but it's
// best to be explicit.
hepProgramBuilder.addMatchOrder(HepMatchOrder.DEPTH_FIRST);
// ----
+ // Rules are disabled if its corresponding value is set to false in
ruleFlags
+ // construct filtered BASIC_RULES, FILTER_PUSHDOWN_RULES,
PROJECT_PUSHDOWN_RULES, PRUNE_RULES
+ List<RelOptRule> basicRules;
+ List<RelOptRule> filterPushdownRules;
+ List<RelOptRule> projectPushdownRules;
+ List<RelOptRule> pruneRules;
+ if (options == null || noRulesSkipped(options)) {
+ basicRules = PinotQueryRuleSets.BASIC_RULES;
+ filterPushdownRules = PinotQueryRuleSets.FILTER_PUSHDOWN_RULES;
+ projectPushdownRules = PinotQueryRuleSets.PROJECT_PUSHDOWN_RULES;
+ pruneRules = PinotQueryRuleSets.PRUNE_RULES;
+ } else {
+ basicRules = filterRuleList(PinotQueryRuleSets.BASIC_RULES, options);
+ filterPushdownRules =
filterRuleList(PinotQueryRuleSets.FILTER_PUSHDOWN_RULES, options);
+ projectPushdownRules =
filterRuleList(PinotQueryRuleSets.PROJECT_PUSHDOWN_RULES, options);
+ pruneRules = filterRuleList(PinotQueryRuleSets.PRUNE_RULES, options);
+ }
+
+
// Run the Calcite CORE rules using 1 HepInstruction per rule. We use 1
HepInstruction per rule for simplicity:
// the rules used here can rest assured that they are the only ones
evaluated in a dedicated graph-traversal.
- for (RelOptRule relOptRule : PinotQueryRuleSets.BASIC_RULES) {
- hepProgramBuilder.addRuleInstance(relOptRule);
+ for (RelOptRule relOptRule : basicRules) {
+ hepProgramBuilder.addRuleInstance(relOptRule);
}
// ----
// Pushdown filters using a single HepInstruction.
-
hepProgramBuilder.addRuleCollection(PinotQueryRuleSets.FILTER_PUSHDOWN_RULES);
+ hepProgramBuilder.addRuleCollection(filterPushdownRules);
// Pushdown projects after first filter pushdown to minimize projected
columns.
-
hepProgramBuilder.addRuleCollection(PinotQueryRuleSets.PROJECT_PUSHDOWN_RULES);
+ hepProgramBuilder.addRuleCollection(projectPushdownRules);
// Pushdown filters again since filter should be pushed down at the lowest
level, after project pushdown.
-
hepProgramBuilder.addRuleCollection(PinotQueryRuleSets.FILTER_PUSHDOWN_RULES);
+ hepProgramBuilder.addRuleCollection(filterPushdownRules);
// ----
// Prune duplicate/unnecessary nodes using a single HepInstruction.
// TODO: We can consider using HepMatchOrder.TOP_DOWN if we find cases
where it would help.
- hepProgramBuilder.addRuleCollection(PinotQueryRuleSets.PRUNE_RULES);
+ hepProgramBuilder.addRuleCollection(pruneRules);
return hepProgramBuilder.build();
}
+ // util func to check no rules are skipped
+ private static boolean noRulesSkipped(Map<String, String> options) {
+ for (Map.Entry<String, String> configEntry : options.entrySet()) {
+ if
(configEntry.getKey().startsWith(CommonConstants.Broker.PLANNER_RULE_SKIP)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Filter static RuleSet according to query options
+ * The filtering is done via checking query option with
+ * key returning from {@link CommonConstants.Broker}.skipRule(rule
description).
+ *
+ * @param rules static list of rules
+ * @param options query options
+ * @return filtered list of rules
+ */
+ private static List<RelOptRule> filterRuleList(List<RelOptRule> rules,
Map<String, String> options) {
+ List<RelOptRule> filteredRules = new ArrayList<>();
+ for (RelOptRule relOptRule : rules) {
+ String ruleName = relOptRule.toString();
+ if (isRuleSkipped(ruleName, options)) {
+ continue;
+ }
+ filteredRules.add(relOptRule);
+ }
+ return filteredRules;
+ }
+
+ /**
+ * Whether a rule is skipped, rules not skipped by default
+ * @param ruleName description of the rule
+ * @param skipMap query skipMap
+ * @return false if corresponding key is not in skipMap or the value is
"false", else true
+ */
+ private static boolean isRuleSkipped(String ruleName, Map<String, String>
skipMap) {
+ // can put rule-specific default behavior here
+ return Boolean.parseBoolean(
+ skipMap.getOrDefault(
+ CommonConstants.Broker.skipRule(ruleName), Boolean.FALSE.toString()
+ )
Review Comment:
(minor)
```suggestion
return
Boolean.parseBoolean(skipMap.get(CommonConstants.Broker.skipRule(ruleName));
```
--
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]
