vvivekiyer commented on code in PR #10315:
URL: https://github.com/apache/pinot/pull/10315#discussion_r1113436552
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/hint/PinotHintStrategyTable.java:
##########
@@ -16,19 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.pinot.query.planner.hints;
-
-import org.apache.calcite.rel.hint.RelHint;
-
+package org.apache.calcite.rel.hint;
/**
- * Provide certain relational hint to query planner for better optimization.
+ * Default hint strategy set for Pinot query.
*/
-public class PinotRelationalHints {
- public static final RelHint AGG_INTERMEDIATE_STAGE =
RelHint.builder("AGG_INTERMEDIATE_STAGE").build();
- public static final RelHint AGG_LEAF_STAGE =
RelHint.builder("AGG_LEAF_STAGE").build();
+public class PinotHintStrategyTable {
+ public static final String INTERNAL_AGG_INTERMEDIATE_STAGE =
"aggIntermediateStage";
+ public static final String INTERNAL_AGG_FINAL_STAGE = "aggFinalStage";
Review Comment:
nit: can we convert this to a static class for each hint? That way we'll be
able to colocate all properties for each hint like name, HintPredicate, Options
Checker, etc?
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java:
##########
@@ -83,8 +83,8 @@ public boolean matches(RelOptRuleCall call) {
}
if (call.rel(0) instanceof Aggregate) {
Aggregate agg = call.rel(0);
- return !agg.getHints().contains(PinotRelationalHints.AGG_LEAF_STAGE)
- &&
!agg.getHints().contains(PinotRelationalHints.AGG_INTERMEDIATE_STAGE);
+ return !agg.getHints().contains(AggregateNode.INTERMEDIATE_STAGE_HINT)
Review Comment:
I think we need to define what would happen in the following cases:
1. User Hint passed is both `aggFinalStage` and `aggIntermediateStage`
2. User hint passed is only `aggFinalStage`
3. User hint passed is only `aggIntermediateStage`
Should we also consider adding tests for each of these cases in
`ResourceBasedQueryPlansTest.java`?
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/hint/PinotHintStrategyTable.java:
##########
@@ -16,19 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.pinot.query.planner.hints;
-
-import org.apache.calcite.rel.hint.RelHint;
-
+package org.apache.calcite.rel.hint;
/**
- * Provide certain relational hint to query planner for better optimization.
+ * Default hint strategy set for Pinot query.
*/
-public class PinotRelationalHints {
- public static final RelHint AGG_INTERMEDIATE_STAGE =
RelHint.builder("AGG_INTERMEDIATE_STAGE").build();
- public static final RelHint AGG_LEAF_STAGE =
RelHint.builder("AGG_LEAF_STAGE").build();
+public class PinotHintStrategyTable {
+ public static final String INTERNAL_AGG_INTERMEDIATE_STAGE =
"aggIntermediateStage";
Review Comment:
Basic question - if the user passes a hint that contains both or one of the
`aggIntermediateStage` and `aggFinalStage` hints, wouldn't we completely skip
applying the PinotAggregateExchangeNodeInsertRule RelOptRule because of the
`PinotAggregateExchangeNodeInsertRule.matches` function will evaluate to false?
Is this the expected behavior?
--
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]
