gortiz commented on code in PR #14664:
URL: https://github.com/apache/pinot/pull/14664#discussion_r1889890059
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java:
##########
@@ -83,48 +85,148 @@
* - COUNT(*)__LEAF produces TUPLE[ SUM(1), GROUP_BY_KEY ]
* - COUNT(*)__FINAL produces TUPLE[ SUM(COUNT(*)__LEAF), GROUP_BY_KEY ]
*/
-public class PinotAggregateExchangeNodeInsertRule extends RelOptRule {
- public static final PinotAggregateExchangeNodeInsertRule INSTANCE =
- new
PinotAggregateExchangeNodeInsertRule(PinotRuleUtils.PINOT_REL_FACTORY);
-
- public PinotAggregateExchangeNodeInsertRule(RelBuilderFactory factory) {
- // NOTE: Explicitly match for LogicalAggregate because after applying the
rule, LogicalAggregate is replaced with
- // PinotLogicalAggregate, and the rule won't be applied again.
- super(operand(LogicalAggregate.class, any()), factory, null);
+public class PinotAggregateExchangeNodeInsertRule {
+
+ public static class SortProjectAggregate extends RelOptRule {
+ public static final SortProjectAggregate INSTANCE = new
SortProjectAggregate(PinotRuleUtils.PINOT_REL_FACTORY);
+
+ private SortProjectAggregate(RelBuilderFactory factory) {
+ // NOTE: Explicitly match for LogicalAggregate because after applying
the rule, LogicalAggregate is replaced with
+ // PinotLogicalAggregate, and the rule won't be applied again.
+ super(operand(Sort.class, operand(Project.class,
operand(LogicalAggregate.class, any()))), factory, null);
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call) {
Review Comment:
nit:
Calcite we have `onMatch` and `matches` methods. This method seems to be
doing both. I think it would be better to override `matches` to do not apply
the rule in all the cases that are filtered out here.
For the final query is the same and we would need to repeat some code, but
the advantage of using _matches_ is that if try to debug which rules apply
(using MarkerFilter, as explained in
https://youtu.be/_phzRNCWJfw?si=hH9ukXAS2Iml11nq&t=331).
I've just created https://github.com/apache/pinot/pull/14680 to do not
forget how to enable these logs.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java:
##########
@@ -83,48 +85,148 @@
* - COUNT(*)__LEAF produces TUPLE[ SUM(1), GROUP_BY_KEY ]
* - COUNT(*)__FINAL produces TUPLE[ SUM(COUNT(*)__LEAF), GROUP_BY_KEY ]
*/
-public class PinotAggregateExchangeNodeInsertRule extends RelOptRule {
- public static final PinotAggregateExchangeNodeInsertRule INSTANCE =
- new
PinotAggregateExchangeNodeInsertRule(PinotRuleUtils.PINOT_REL_FACTORY);
-
- public PinotAggregateExchangeNodeInsertRule(RelBuilderFactory factory) {
- // NOTE: Explicitly match for LogicalAggregate because after applying the
rule, LogicalAggregate is replaced with
- // PinotLogicalAggregate, and the rule won't be applied again.
- super(operand(LogicalAggregate.class, any()), factory, null);
+public class PinotAggregateExchangeNodeInsertRule {
+
+ public static class SortProjectAggregate extends RelOptRule {
+ public static final SortProjectAggregate INSTANCE = new
SortProjectAggregate(PinotRuleUtils.PINOT_REL_FACTORY);
+
+ private SortProjectAggregate(RelBuilderFactory factory) {
+ // NOTE: Explicitly match for LogicalAggregate because after applying
the rule, LogicalAggregate is replaced with
+ // PinotLogicalAggregate, and the rule won't be applied again.
+ super(operand(Sort.class, operand(Project.class,
operand(LogicalAggregate.class, any()))), factory, null);
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call) {
+ // Apply this rule for group-by queries with enable group trim hint.
+ LogicalAggregate aggRel = call.rel(2);
+ if (aggRel.getGroupSet().isEmpty()) {
+ return;
+ }
+ Map<String, String> hintOptions =
+ PinotHintStrategyTable.getHintOptions(aggRel.getHints(),
PinotHintOptions.AGGREGATE_HINT_OPTIONS);
+ if (hintOptions == null || !Boolean.parseBoolean(
+
hintOptions.get(PinotHintOptions.AggregateOptions.ENABLE_GROUP_TRIM))) {
+ return;
+ }
+
+ Sort sortRel = call.rel(0);
+ Project projectRel = call.rel(1);
+ List<RexNode> projects = projectRel.getProjects();
+ List<RelFieldCollation> collations =
sortRel.getCollation().getFieldCollations();
+ if (collations.isEmpty()) {
+ // Cannot enable group trim without sort key.
Review Comment:
I understand this is required for [within
segment](https://docs.pinot.apache.org/users/user-guide-query/query-syntax/grouping-algorithm#within-segment)
trimming right? Is it an actual requirement for cross segment trimming?
Anyway, I'm not sure if we should make this kind of decisions here. Couldn't
we populate the fields and let the actual operator decide whether an
optimization can be applied or not?
More specifically: Don't you think it may be interesting to keep the limit
even if we don't have collations?
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java:
##########
@@ -83,48 +85,148 @@
* - COUNT(*)__LEAF produces TUPLE[ SUM(1), GROUP_BY_KEY ]
* - COUNT(*)__FINAL produces TUPLE[ SUM(COUNT(*)__LEAF), GROUP_BY_KEY ]
*/
-public class PinotAggregateExchangeNodeInsertRule extends RelOptRule {
- public static final PinotAggregateExchangeNodeInsertRule INSTANCE =
- new
PinotAggregateExchangeNodeInsertRule(PinotRuleUtils.PINOT_REL_FACTORY);
-
- public PinotAggregateExchangeNodeInsertRule(RelBuilderFactory factory) {
- // NOTE: Explicitly match for LogicalAggregate because after applying the
rule, LogicalAggregate is replaced with
- // PinotLogicalAggregate, and the rule won't be applied again.
- super(operand(LogicalAggregate.class, any()), factory, null);
+public class PinotAggregateExchangeNodeInsertRule {
+
+ public static class SortProjectAggregate extends RelOptRule {
Review Comment:
Although the javadoc in the main class applies to all rules, it would be
cool to add a javadoc for each rule to explain what they do (and maybe add an
example)
--
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]
