Jackie-Jiang commented on code in PR #15999:
URL: https://github.com/apache/pinot/pull/15999#discussion_r2151095735
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -133,9 +136,9 @@ public class QueryEnvironment {
private final TypeFactory _typeFactory = new TypeFactory();
private final FrameworkConfig _config;
private final CalciteCatalogReader _catalogReader;
- private final HepProgram _optProgram;
private final Config _envConfig;
private final PinotCatalog _catalog;
+ private HepProgram _optProgram;
Review Comment:
(minor) Revert this change
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -163,6 +167,15 @@ public QueryEnvironment(String database, TableCache
tableCache, @Nullable Worker
*/
private PlannerContext getPlannerContext(SqlNodeAndOptions
sqlNodeAndOptions) {
WorkerManager workerManager = getWorkerManager(sqlNodeAndOptions);
+ Map<String, String> options = sqlNodeAndOptions.getOptions();
+ HepProgram optProgram = _optProgram;
+ if (options != null && !options.isEmpty()) {
+ Set<String> skipRuleSet = QueryOptionsUtils.getSkipPlannerRules(options);
+ if (skipRuleSet != null && !skipRuleSet.isEmpty()) {
Review Comment:
(minor)
```suggestion
if (CollectionUtils.isNotEmpty(skipRuleSet)) {
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java:
##########
@@ -3476,7 +3476,7 @@ public void testExplainPlanQueryV2()
+ " PinotLogicalTableScan(table=[[default, mytable]])\n");
assertEquals(response1Json.get("rows").get(0).get(2).asText(), "Rule
Execution Times\n"
+ "Rule: AggregateProjectMergeRule -> Time:*\n"
- + "Rule: Project -> Time:*\n"
+ + "Rule: EvaluateProjectLiteralRule -> Time:*\n"
Review Comment:
(minor) Consider changing the `HashMap` into a `LinkedHashMap` to preserve
the order of rule application, and also make this deterministic
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -163,6 +167,15 @@ public QueryEnvironment(String database, TableCache
tableCache, @Nullable Worker
*/
private PlannerContext getPlannerContext(SqlNodeAndOptions
sqlNodeAndOptions) {
WorkerManager workerManager = getWorkerManager(sqlNodeAndOptions);
+ Map<String, String> options = sqlNodeAndOptions.getOptions();
+ HepProgram optProgram = _optProgram;
+ if (options != null && !options.isEmpty()) {
Review Comment:
(minor)
```suggestion
if (MapUtils.isNotEmpty(options)) {
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -664,6 +667,51 @@ 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 FILTER_INTO_JOIN = "FilterIntoJoinRule";
+ 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";
Review Comment:
(minor) I'd suggest removing the `PINOT` from the key so that key and value
can match. In the future if we need to modify a Calcite rule, we won't need to
also modify these constants
--
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]
