You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "walterddr (via GitHub)" <gi...@apache.org> on 2023/07/05 23:30:28 UTC
[GitHub] [pinot] walterddr opened a new pull request, #11036: [multistage] clean up aggregate v1 work
walterddr opened a new pull request, #11036:
URL: https://github.com/apache/pinot/pull/11036
Summary
===
clean up hint-based aggregate node representation, see #11034
Detail
===
- consolidate mode / agg / hint all into just one AggType enum
- remove all unnecessary castings and boolean extractions.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #11036: [multistage] clean up aggregate v1 function for v2 planner
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11036:
URL: https://github.com/apache/pinot/pull/11036#issuecomment-1622714420
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11036](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c0bdbee) into [master](https://app.codecov.io/gh/apache/pinot/commit/f7a076496ae6e07a42207cb2c978dc74562cf0cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f7a0764) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11036 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2200 2145 -55
Lines 118829 116240 -2589
Branches 18023 17694 -329
=========================================
Hits 137 137
+ Misses 118672 116083 -2589
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (+<0.01%)` | :arrow_up: |
| unittests2temurin17 | `?` | |
| unittests2temurin20 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...gregation/function/AggregationFunctionFactory.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BZ2dyZWdhdGlvbkZ1bmN0aW9uRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...query/reduce/function/InternalReduceFunctions.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvZnVuY3Rpb24vSW50ZXJuYWxSZWR1Y2VGdW5jdGlvbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...uery/planner/physical/DispatchablePlanVisitor.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9waHlzaWNhbC9EaXNwYXRjaGFibGVQbGFuVmlzaXRvci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...he/pinot/query/planner/plannode/AggregateNode.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9wbGFubm9kZS9BZ2dyZWdhdGVOb2RlLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...inot/query/runtime/operator/AggregateOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9BZ2dyZWdhdGVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...untime/operator/MultistageAggregationExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aXN0YWdlQWdncmVnYXRpb25FeGVjdXRvci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ry/runtime/operator/MultistageGroupByExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aXN0YWdlR3JvdXBCeUV4ZWN1dG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../pinot/query/runtime/plan/PhysicalPlanVisitor.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL1BoeXNpY2FsUGxhblZpc2l0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...che/pinot/segment/spi/AggregationFunctionType.java](https://app.codecov.io/gh/apache/pinot/pull/11036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL0FnZ3JlZ2F0aW9uRnVuY3Rpb25UeXBlLmphdmE=) | `0.00% <0.00%> (ø)` | |
... and [59 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11036/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11036: [multistage] clean up aggregate v1 function for v2 planner
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11036:
URL: https://github.com/apache/pinot/pull/11036#discussion_r1254804032
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -29,12 +29,28 @@ public class PinotHintOptions {
public static final String AGGREGATE_HINT_OPTIONS = "aggOptions";
public static final String JOIN_HINT_OPTIONS = "joinOptions";
+ /**
+ * Hint to denote that the aggregation node is the final aggregation stage which extracts the final result.
+ */
+ public static final String INTERNAL_AGG_OPTIONS = "aggOptionsInternal";
+
private PinotHintOptions() {
// do not instantiate.
}
+ public static class InternalAggregateOptions {
+ public static final String AGG_TYPE = "agg_type";
+ public enum AggType {
+ DIRECT,
Review Comment:
Can you add more comments on the type to distinguish `DIRECT` and `LEAF`?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/plannode/AggregateNode.java:
##########
@@ -115,29 +72,13 @@ public List<RelHint> getRelHints() {
return _relHints;
}
- public AggregationStage getAggregationStage() {
- return _aggregationStage;
- }
-
- public boolean isTreatIntermediateStageAsLeaf() {
- return _treatIntermediateStageAsLeaf;
- }
-
- public boolean isFinalStage() {
- return _aggregationStage == AggregationStage.FINAL;
- }
-
- public boolean isIntermediateStage() {
- return _aggregationStage == AggregationStage.INTERMEDIATE;
- }
-
- public boolean isLeafStage() {
- return _aggregationStage == AggregationStage.LEAF;
+ public PinotHintOptions.InternalAggregateOptions.AggType getAggType() {
+ return _aggType;
}
@Override
public String explain() {
- return "AGGREGATE";
+ return "AGGREGATE" + _aggType;
Review Comment:
`"AGGREGATE_" + _aggType` ?
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -29,12 +29,28 @@ public class PinotHintOptions {
public static final String AGGREGATE_HINT_OPTIONS = "aggOptions";
public static final String JOIN_HINT_OPTIONS = "joinOptions";
+ /**
+ * Hint to denote that the aggregation node is the final aggregation stage which extracts the final result.
+ */
+ public static final String INTERNAL_AGG_OPTIONS = "aggOptionsInternal";
+
private PinotHintOptions() {
// do not instantiate.
}
+ public static class InternalAggregateOptions {
+ public static final String AGG_TYPE = "agg_type";
+ public enum AggType {
Review Comment:
suggest add boolean methods like: isFinal(), isLeaf()... for comparison simplicity.
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java:
##########
@@ -248,12 +227,12 @@ private RelNode makeNewIntermediateAgg(RelOptRuleCall ruleCall, Aggregate oldAgg
*/
private static void convertIntermediateAggCall(RexBuilder rexBuilder, Aggregate oldAggRel, int oldCallIndex,
AggregateCall oldCall, List<AggregateCall> newCalls, Map<AggregateCall, RexNode> aggCallMapping,
- boolean isLeafStageAggregationPresent, List<Integer> argList, PinotLogicalExchange exchange) {
+ AggType aggType, List<Integer> argList, PinotLogicalExchange exchange) {
final int nGroups = oldAggRel.getGroupCount();
final SqlAggFunction oldAggregation = oldCall.getAggregation();
List<Integer> newArgList;
- if (isLeafStageAggregationPresent) {
+ if (AggType.INTERMEDIATE.equals(aggType) || AggType.FINAL.equals(aggType)) {
Review Comment:
worth a static method `isLeafStageAggregation()` for this?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #11036: [multistage] clean up aggregate v1 function for v2 planner
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11036:
URL: https://github.com/apache/pinot/pull/11036#discussion_r1254824434
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java:
##########
@@ -248,12 +227,12 @@ private RelNode makeNewIntermediateAgg(RelOptRuleCall ruleCall, Aggregate oldAgg
*/
private static void convertIntermediateAggCall(RexBuilder rexBuilder, Aggregate oldAggRel, int oldCallIndex,
AggregateCall oldCall, List<AggregateCall> newCalls, Map<AggregateCall, RexNode> aggCallMapping,
- boolean isLeafStageAggregationPresent, List<Integer> argList, PinotLogicalExchange exchange) {
+ AggType aggType, List<Integer> argList, PinotLogicalExchange exchange) {
final int nGroups = oldAggRel.getGroupCount();
final SqlAggFunction oldAggregation = oldCall.getAggregation();
List<Integer> newArgList;
- if (isLeafStageAggregationPresent) {
+ if (AggType.INTERMEDIATE.equals(aggType) || AggType.FINAL.equals(aggType)) {
Review Comment:
yeah i tried. this is not the same as "isLeafStage" b/c the treatment during process input block and produce output block as different. so it has to be directly comparing the types
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #11036: [multistage] clean up aggregate v1 function for v2 planner
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #11036:
URL: https://github.com/apache/pinot/pull/11036
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #11036: [multistage] clean up aggregate v1 function for v2 planner
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11036:
URL: https://github.com/apache/pinot/pull/11036#discussion_r1254825251
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -29,12 +29,28 @@ public class PinotHintOptions {
public static final String AGGREGATE_HINT_OPTIONS = "aggOptions";
public static final String JOIN_HINT_OPTIONS = "joinOptions";
+ /**
+ * Hint to denote that the aggregation node is the final aggregation stage which extracts the final result.
+ */
+ public static final String INTERNAL_AGG_OPTIONS = "aggOptionsInternal";
+
private PinotHintOptions() {
// do not instantiate.
}
+ public static class InternalAggregateOptions {
+ public static final String AGG_TYPE = "agg_type";
+ public enum AggType {
Review Comment:
will do in the next iteration. having boolean method is not that useful IMO b/c there's still 4 combination
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/plannode/AggregateNode.java:
##########
@@ -115,29 +72,13 @@ public List<RelHint> getRelHints() {
return _relHints;
}
- public AggregationStage getAggregationStage() {
- return _aggregationStage;
- }
-
- public boolean isTreatIntermediateStageAsLeaf() {
- return _treatIntermediateStageAsLeaf;
- }
-
- public boolean isFinalStage() {
- return _aggregationStage == AggregationStage.FINAL;
- }
-
- public boolean isIntermediateStage() {
- return _aggregationStage == AggregationStage.INTERMEDIATE;
- }
-
- public boolean isLeafStage() {
- return _aggregationStage == AggregationStage.LEAF;
+ public PinotHintOptions.InternalAggregateOptions.AggType getAggType() {
+ return _aggType;
}
@Override
public String explain() {
- return "AGGREGATE";
+ return "AGGREGATE" + _aggType;
Review Comment:
good idea
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #11036: [multistage] clean up aggregate v1 function for v2 planner
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11036:
URL: https://github.com/apache/pinot/pull/11036#discussion_r1253755083
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -29,12 +29,29 @@ public class PinotHintOptions {
public static final String AGGREGATE_HINT_OPTIONS = "aggOptions";
public static final String JOIN_HINT_OPTIONS = "joinOptions";
+ /**
+ * Hint to denote that the aggregation node is the final aggregation stage which extracts the final result.
+ */
+ public static final String INTERNAL_AGG_OPTIONS = "aggOptionsInternal";
+
private PinotHintOptions() {
// do not instantiate.
}
+ public static class InternalAggregateOptions {
+ public static final String AGG_TYPE = "agg_type";
+ public enum AggType {
+ DIRECT,
+ LEAF,
+ INTERMEDIATE,
+ FINAL,
+ REDUCE // NOT SUPPORTED
+ }
Review Comment:
these are the only necessary MODE we needed
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11036: [multistage] clean up aggregate v1 function for v2 planner
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11036:
URL: https://github.com/apache/pinot/pull/11036#discussion_r1254807898
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java:
##########
@@ -248,12 +227,12 @@ private RelNode makeNewIntermediateAgg(RelOptRuleCall ruleCall, Aggregate oldAgg
*/
private static void convertIntermediateAggCall(RexBuilder rexBuilder, Aggregate oldAggRel, int oldCallIndex,
AggregateCall oldCall, List<AggregateCall> newCalls, Map<AggregateCall, RexNode> aggCallMapping,
- boolean isLeafStageAggregationPresent, List<Integer> argList, PinotLogicalExchange exchange) {
+ AggType aggType, List<Integer> argList, PinotLogicalExchange exchange) {
final int nGroups = oldAggRel.getGroupCount();
final SqlAggFunction oldAggregation = oldCall.getAggregation();
List<Integer> newArgList;
- if (isLeafStageAggregationPresent) {
+ if (AggType.INTERMEDIATE.equals(aggType) || AggType.FINAL.equals(aggType)) {
Review Comment:
worth a static method `isLeafStageAggregation()` for this?
or `if (aggType.isIntermediate() || aggType.isFinal()) {`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org