You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/10/11 22:24:10 UTC
[GitHub] [pinot] agavra opened a new pull request, #9570: [multistage] fix GROUP BY with no aggregate functions
agavra opened a new pull request, #9570:
URL: https://github.com/apache/pinot/pull/9570
This PR fixes a bug where the v1 engine doesn't properly handle (at the server level) queries that have a GROUP BY but no aggregate functions or explicitly use SELECT DISTINCT.
Pretty straightforward:
- if encountering a `GROUP BY` with no aggregate, apply the appropriate rewriter
- add a reduce operator to convert the intermediate format to the expected format
- change the query select list to match (no select expressions other than what should be returned)
- two minor refactors
Testing done: added more tests in QueryRunnerTest
--
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 #9570: [multistage] fix GROUP BY with no aggregate functions
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9570:
URL: https://github.com/apache/pinot/pull/9570#discussion_r993952677
##########
pinot-core/src/main/java/org/apache/pinot/core/query/distinct/DistinctTable.java:
##########
@@ -406,4 +407,24 @@ public static DistinctTable fromByteBuffer(ByteBuffer byteBuffer)
}
return new DistinctTable(dataSchema, records);
}
+
+ public ResultTable reduceToResultTable() {
+ List<Object[]> rows = new ArrayList<>(size());
+ DataSchema dataSchema = getDataSchema();
+ ColumnDataType[] columnDataTypes = dataSchema.getColumnDataTypes();
+ int numColumns = columnDataTypes.length;
+ Iterator<Record> iterator = getFinalResult();
+ while (iterator.hasNext()) {
+ Object[] values = iterator.next().getValues();
+ Object[] row = new Object[numColumns];
+ for (int i = 0; i < numColumns; i++) {
+ Object value = values[i];
+ if (value != null) {
+ row[i] = columnDataTypes[i].convertAndFormat(value);
+ }
+ }
+ rows.add(row);
+ }
+ return new ResultTable(dataSchema, rows);
+ }
Review Comment:
let's remove this from DistinctTable this seems to be a pure util function.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/DistinctDataTableReducer.java:
##########
@@ -94,27 +92,7 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
for (DistinctTable distinctTable : nonEmptyDistinctTables) {
mainDistinctTable.mergeTable(distinctTable);
}
- brokerResponseNative.setResultTable(reduceToResultTable(mainDistinctTable));
+ brokerResponseNative.setResultTable(mainDistinctTable.reduceToResultTable());
}
}
-
- private ResultTable reduceToResultTable(DistinctTable distinctTable) {
Review Comment:
the goal is to move this logic to be use for both the v1 broker reduce and the v2 leaf stage. thus we can apply the same logic from `AggregationResultBlock`
- change `DistinctResultBlock` method `getRow(QueryContext )` and `getDataTble(QueryContext)` method to take into account the flag `queryContext.isServerReturnFinalResult()`
- if running using v2 engine. mark `queryContext.setServerReturnFinalResult(true)` when aggregate function is empty
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/QueryRunner.java:
##########
@@ -125,9 +124,21 @@ public void processQuery(DistributedStagePlan distributedStagePlan, ExecutorServ
MailboxSendNode sendNode = (MailboxSendNode) distributedStagePlan.getStageRoot();
StageMetadata receivingStageMetadata = distributedStagePlan.getMetadataMap().get(sendNode.getReceiverStageId());
+
+ BaseOperator<TransferableBlock> inputOperator
+ = new LeafStageTransferableBlockOperator(serverQueryResults, sendNode.getDataSchema());
+
+ // hack this for now, we should be creating a "real" operator tree - this makes sure that we properly
+ // handle the SELECT DISTINCT rewrite
+ AggregationFunction[] aggFunctions = serverQueryRequests.get(0).getQueryContext().getAggregationFunctions();
+ if (aggFunctions != null && aggFunctions[0] instanceof DistinctAggregationFunction) {
+ inputOperator = new LeafDistinctCompatibilityOperator(inputOperator,
+ serverQueryRequests.get(0).getQueryContext(), distributedStagePlan.getStageRoot().getDataSchema());
+ }
+
MailboxSendOperator mailboxSendOperator =
new MailboxSendOperator(_mailboxService, sendNode.getDataSchema(),
- new LeafStageTransferableBlockOperator(serverQueryResults, sendNode.getDataSchema()),
+ inputOperator,
Review Comment:
instead of adding special handling here. use `queryContext.setServerReturnFinalResult(true)` in the `ServerRequestUtil`
--
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] agavra commented on a diff in pull request #9570: [multistage] fix GROUP BY with no aggregate functions
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9570:
URL: https://github.com/apache/pinot/pull/9570#discussion_r992829535
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/utils/ServerRequestUtils.java:
##########
@@ -182,12 +186,15 @@ private static void walkStageTree(StageNode node, PinotQuery pinotQuery, TableTy
pinotQuery.setSelectList(CalciteRexExpressionParser.overwriteSelectList(
((ProjectNode) node).getProjects(), pinotQuery));
} else if (node instanceof AggregateNode) {
- // set agg list
- pinotQuery.setSelectList(CalciteRexExpressionParser.addSelectList(pinotQuery.getSelectList(),
- ((AggregateNode) node).getAggCalls(), pinotQuery));
// set group-by list
pinotQuery.setGroupByList(CalciteRexExpressionParser.convertGroupByList(
((AggregateNode) node).getGroupSet(), pinotQuery));
+ // set agg list - the elements that are selected for an aggregate query
+ // should include all of the group by list followed by the aggregate functions
+ // if other columns are selected upstream they will be projected out in a
+ // dedicated ProjectNode
+ pinotQuery.setSelectList(CalciteRexExpressionParser.addSelectList(pinotQuery.getGroupByList(),
+ ((AggregateNode) node).getAggCalls(), pinotQuery));
Review Comment:
yes, that's why we couldn't use the `NonAggregationGroupByToDistinctQueryRewriter`. But once we use `NonAggregationGroupByToDistinctQueryRewriter` we need to add `LeafDistinctCompatibilityOperator` to interpret the results.
--
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 #9570: [multistage] fix GROUP BY with no aggregate functions
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9570:
URL: https://github.com/apache/pinot/pull/9570#issuecomment-1275399082
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9570?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9570](https://codecov.io/gh/apache/pinot/pull/9570?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4152963) into [master](https://codecov.io/gh/apache/pinot/commit/03121cf4553ebbd0251df028a55387f96db0b619?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (03121cf) will **decrease** coverage by `34.33%`.
> The diff coverage is `25.00%`.
```diff
@@ Coverage Diff @@
## master #9570 +/- ##
=============================================
- Coverage 60.34% 26.01% -34.34%
+ Complexity 5072 44 -5028
=============================================
Files 1919 1921 +2
Lines 102764 102787 +23
Branches 15616 15616
=============================================
- Hits 62017 26744 -35273
- Misses 36089 73352 +37263
+ Partials 4658 2691 -1967
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `26.01% <25.00%> (?)` | |
| integration2 | `?` | |
| unittests1 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/query/runtime/QueryRunner.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9RdWVyeVJ1bm5lci5qYXZh) | `0.00% <0.00%> (-78.95%)` | :arrow_down: |
| [...untime/leaf/LeafDistinctCompatibilityOperator.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9sZWFmL0xlYWZEaXN0aW5jdENvbXBhdGliaWxpdHlPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ntime/leaf/LeafStageTransferableBlockOperator.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9sZWFmL0xlYWZTdGFnZVRyYW5zZmVyYWJsZUJsb2NrT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [.../pinot/query/runtime/utils/ServerRequestUtils.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS91dGlscy9TZXJ2ZXJSZXF1ZXN0VXRpbHMuamF2YQ==) | `0.00% <0.00%> (-82.25%)` | :arrow_down: |
| [...pache/pinot/core/query/distinct/DistinctTable.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9kaXN0aW5jdC9EaXN0aW5jdFRhYmxlLmphdmE=) | `64.50% <93.33%> (-26.86%)` | :arrow_down: |
| [...ot/core/query/reduce/DistinctDataTableReducer.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvRGlzdGluY3REYXRhVGFibGVSZWR1Y2VyLmphdmE=) | `62.06% <100.00%> (-33.39%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1273 more](https://codecov.io/gh/apache/pinot/pull/9570/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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=The+Apache+Software+Foundation)
--
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] agavra closed pull request #9570: [multistage] fix GROUP BY with no aggregate functions
Posted by GitBox <gi...@apache.org>.
agavra closed pull request #9570: [multistage] fix GROUP BY with no aggregate functions
URL: https://github.com/apache/pinot/pull/9570
--
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 #9570: [multistage] fix GROUP BY with no aggregate functions
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9570:
URL: https://github.com/apache/pinot/pull/9570#discussion_r992827945
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/utils/ServerRequestUtils.java:
##########
@@ -182,12 +186,15 @@ private static void walkStageTree(StageNode node, PinotQuery pinotQuery, TableTy
pinotQuery.setSelectList(CalciteRexExpressionParser.overwriteSelectList(
((ProjectNode) node).getProjects(), pinotQuery));
} else if (node instanceof AggregateNode) {
- // set agg list
- pinotQuery.setSelectList(CalciteRexExpressionParser.addSelectList(pinotQuery.getSelectList(),
- ((AggregateNode) node).getAggCalls(), pinotQuery));
// set group-by list
pinotQuery.setGroupByList(CalciteRexExpressionParser.convertGroupByList(
((AggregateNode) node).getGroupSet(), pinotQuery));
+ // set agg list - the elements that are selected for an aggregate query
+ // should include all of the group by list followed by the aggregate functions
+ // if other columns are selected upstream they will be projected out in a
+ // dedicated ProjectNode
+ pinotQuery.setSelectList(CalciteRexExpressionParser.addSelectList(pinotQuery.getGroupByList(),
+ ((AggregateNode) node).getAggCalls(), pinotQuery));
Review Comment:
this is the main issue yes?
--
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 #9570: [multistage] fix GROUP BY with no aggregate functions
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9570:
URL: https://github.com/apache/pinot/pull/9570#discussion_r992827945
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/utils/ServerRequestUtils.java:
##########
@@ -182,12 +186,15 @@ private static void walkStageTree(StageNode node, PinotQuery pinotQuery, TableTy
pinotQuery.setSelectList(CalciteRexExpressionParser.overwriteSelectList(
((ProjectNode) node).getProjects(), pinotQuery));
} else if (node instanceof AggregateNode) {
- // set agg list
- pinotQuery.setSelectList(CalciteRexExpressionParser.addSelectList(pinotQuery.getSelectList(),
- ((AggregateNode) node).getAggCalls(), pinotQuery));
// set group-by list
pinotQuery.setGroupByList(CalciteRexExpressionParser.convertGroupByList(
((AggregateNode) node).getGroupSet(), pinotQuery));
+ // set agg list - the elements that are selected for an aggregate query
+ // should include all of the group by list followed by the aggregate functions
+ // if other columns are selected upstream they will be projected out in a
+ // dedicated ProjectNode
+ pinotQuery.setSelectList(CalciteRexExpressionParser.addSelectList(pinotQuery.getGroupByList(),
+ ((AggregateNode) node).getAggCalls(), pinotQuery));
Review Comment:
this is the main issue e.g. we should add the group list not the select list, correct?
--
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] agavra commented on pull request #9570: [multistage] fix GROUP BY with no aggregate functions
Posted by GitBox <gi...@apache.org>.
agavra commented on PR #9570:
URL: https://github.com/apache/pinot/pull/9570#issuecomment-1281135955
closing this in favor of #9582
--
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