Support ListAgg WITHIN GROUP clause #13146

xiangfu0 · 2024-05-13T20:18:46Z

Support WITHIN GROUP Clause in MultiStage Engine
- Use PinotLogicalSortExchange for WITHIN GROUP instead of PinotLogicalExchange for leaf data exchange
Support ListAgg aggregation function
Support passing Distinct flag to ListAgg implementation

Limitation:

Only one WITHIN GROUP Clause is supported right now.

Sample query:

SELECT 
    Origin, Dest,
    LISTAGG(distinct Carrier, ', ') WITHIN GROUP (ORDER BY Carrier)
FROM 
    airlineStats
GROUP BY 
    Dest, Origin

Query Plan:

Execution Plan
LogicalProject(Origin=[$1], Dest=[$0], EXPR$2=[$2])
  LogicalAggregate(group=[{0, 1}], EXPR$2=[LISTAGG(DISTINCT $2, $3) WITHIN GROUP ([2])])
    PinotLogicalSortExchange(distribution=[hash[0, 1]], collation=[[2]], isSortOnSender=[false], isSortOnReceiver=[true])
      LogicalProject(Dest=[$28], Origin=[$61], Carrier=[$17], $f3=[_UTF-8', '])
        LogicalTableScan(table=[[default, airlineStats]])

SELECT
    Carrier,
    LISTAGG(DISTINCT CONCAT(Origin, Dest, ' | '), ', ') WITHIN GROUP (ORDER BY Origin, Dest)
FROM airlineStats
GROUP BY Carrier
ORDER BY Carrier

Query Plan

Execution Plan
LogicalSort(sort0=[$0], dir0=[ASC])
  PinotLogicalSortExchange(distribution=[hash], collation=[[0]], isSortOnSender=[false], isSortOnReceiver=[true])
    LogicalAggregate(group=[{0}], EXPR$1=[LISTAGG(DISTINCT $1, $2) WITHIN GROUP ([3, 4])])
      PinotLogicalSortExchange(distribution=[hash[0]], collation=[[3, 4]], isSortOnSender=[false], isSortOnReceiver=[true])
        LogicalProject(Carrier=[$17], $f1=[CONCAT($61, $28, _UTF-8' | ')], $f2=[_UTF-8', '], Origin=[$61], Dest=[$28])
          LogicalTableScan(table=[[default, airlineStats]])

codecov-commenter · 2024-05-13T21:07:33Z

Codecov Report

Attention: Patch coverage is 40.12346% with 97 lines in your changes are missing coverage. Please review.

Project coverage is 62.22%. Comparing base (59551e4) to head (60d2a99).
Report is 479 commits behind head on master.

Files	Patch %	Lines
...ry/aggregation/function/array/ListAggFunction.java	0.00%	51 Missing ⚠️
...el/rules/PinotAggregateExchangeNodeInsertRule.java	61.76%	7 Missing and 6 partials ⚠️
...gregation/function/AggregationFunctionFactory.java	0.00%	12 Missing ⚠️
...gation/function/array/ListAggDistinctFunction.java	0.00%	12 Missing ⚠️
...org/apache/pinot/core/common/ObjectSerDeUtils.java	88.88%	2 Missing and 2 partials ⚠️
...inot/query/runtime/operator/AggregateOperator.java	62.50%	2 Missing and 1 partial ⚠️
...che/pinot/query/planner/logical/RexExpression.java	80.00%	1 Missing ⚠️
...che/pinot/segment/spi/AggregationFunctionType.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13146      +/-   ##
============================================
+ Coverage     61.75%   62.22%   +0.47%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2531      +95     
  Lines        133233   138446    +5213     
  Branches      20636    21428     +792     
============================================
+ Hits          82274    86152    +3878     
- Misses        44911    45866     +955     
- Partials       6048     6428     +380

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`35.22% <40.12%> (-26.49%)`	⬇️
java-21	`62.11% <40.12%> (+0.49%)`	⬆️
skip-bytebuffers-false	`62.19% <40.12%> (+0.45%)`	⬆️
skip-bytebuffers-true	`62.10% <40.12%> (+34.37%)`	⬆️
temurin	`62.22% <40.12%> (+0.47%)`	⬆️
unittests	`62.22% <40.12%> (+0.47%)`	⬆️
unittests1	`46.75% <40.12%> (-0.14%)`	⬇️
unittests2	`27.87% <0.00%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jackie-Jiang

We should generalize the query flow for aggregate with distinct or within group order by. It should be modeled as projection/distinct on the leaf stage instead of aggregation. The aggregation should only be applied on the intermediate stage, which is similar to the window function handling logic

pinot-common/src/main/java/org/apache/pinot/common/request/context/FunctionContext.java

...re/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ListAggFunction.java

pinot-query-planner/src/main/java/org/apache/pinot/query/planner/plannode/AggregateNode.java

...re/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ListAggFunction.java

xiangfu0 · 2024-05-19T12:33:06Z

We should generalize the query flow for aggregate with distinct or within group order by. It should be modeled as projection/distinct on the leaf stage instead of aggregation. The aggregation should only be applied on the intermediate stage, which is similar to the window function handling logic

Do you mean We should have a new rule or reuse existing rule to modify the query plan?

xiangfu0 · 2024-05-19T12:38:32Z

We should generalize the query flow for aggregate with distinct or within group order by. It should be modeled as projection/distinct on the leaf stage instead of aggregation. The aggregation should only be applied on the intermediate stage, which is similar to the window function handling logic

I think the distinct will still happen in both leaf and intermediate stage, as there could still be duplicates across leaf nodes.

xiangfu0 · 2024-05-20T07:54:28Z

Simplified the code to for both collation and distinct handling.

...re/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ListAggFunction.java

Jackie-Jiang

The difference for ListAggFunction and ArrayAggString/ArrayAggDistinctStringFunction is just the extract final results part. Since you need to do special handling on v2 side, is it possible to directly use the ArrayAgg? If not, probably extend that and only override the extract final results

...re/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ListAggFunction.java

xiangfu0 · 2024-05-21T01:14:39Z

The difference for ListAggFunction and ArrayAggString/ArrayAggDistinctStringFunction is just the extract final results part. Since you need to do special handling on v2 side, is it possible to directly use the ArrayAgg? If not, probably extend that and only override the extract final results

ArrayAgg doesn't support ORDER BY so the merger/extractFinal at intermediate/final stage doesn't honer the ordering.

Jackie-Jiang · 2024-05-21T06:38:05Z

...c/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java

Let's not change this signature, and use argument to pass distinct similar to ARRAYAGG. In the future, we don't want to handle distinct logic within the aggregation function

Reverted getAggregationFunction signature, it means we need to have special handle for LISTAGG to append isDistinct to the third argument.

This is due to LISTAGG is registered in SqlStdOperatorTable, so the sql syntax cannot be changed, usage can only be LISTAGG(distinct expr, ' | ') not LISTAGG(expr, ' | ', true).

This is the error for the query:

SELECT Origin, Dest, LISTAGG(Carrier, ', ', 'true') WITHIN GROUP (ORDER BY Carrier) FROM airlineStats GROUP BY Dest, Origin

ProcessingException(errorCode:150, message:SQLParsingError: java.lang.Exception: Unable to find table for this query at org.apache.pinot.controller.api.resources.PinotQueryResource.getMultiStageQueryResponse(PinotQueryResource.java:222) at org.apache.pinot.controller.api.resources.PinotQueryResource.executeSqlQuery(PinotQueryResource.java:179) at org.apache.pinot.controller.api.resources.PinotQueryResource.handlePostSql(PinotQueryResource.java:127) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ... Caused by: java.lang.RuntimeException: Error composing query plan for: SELECT Origin, Dest, LISTAGG(Carrier, ', ', 'true') WITHIN GROUP (ORDER BY Carrier) FROM airlineStats ... Caused by: org.apache.calcite.runtime.CalciteContextException: From line 3, column 5 to line 3, column 34: Invalid number of arguments to function 'LISTAGG'. Was expecting 1 arguments at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490) ... Caused by: org.apache.calcite.sql.validate.SqlValidatorException: Invalid number of arguments to function 'LISTAGG'. Was expecting 1 arguments at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490))

...ain/java/org/apache/pinot/core/query/aggregation/function/array/ListAggDistinctFunction.java

...re/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ListAggFunction.java

Jackie-Jiang

LGTM otherwise

...c/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java

...ain/java/org/apache/pinot/core/query/aggregation/function/array/ListAggDistinctFunction.java

...t-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/AggregateOperator.java

* Support ListAgg WITHIN GROUP clause * fix rowCompartor schema * Change rule to insert a sort for WithInGroup clause * move isDistinct field out from FunctionContext * address comments

xiangfu0 force-pushed the with-in-groupby branch from 191ebfa to 8d78e23 Compare May 13, 2024 20:22

xiangfu0 force-pushed the with-in-groupby branch from 8d78e23 to 49349e8 Compare May 13, 2024 21:35

xiangfu0 added feature multi-stage Related to the multi-stage query engine labels May 13, 2024

xiangfu0 mentioned this pull request May 13, 2024

Support StringJoin aggregation function #13096

Closed

xiangfu0 force-pushed the with-in-groupby branch 4 times, most recently from 07d980d to 574394e Compare May 14, 2024 07:12

xiangfu0 requested review from Jackie-Jiang and gortiz May 14, 2024 08:50

Jackie-Jiang reviewed May 15, 2024

View reviewed changes

xiangfu0 force-pushed the with-in-groupby branch from 574394e to 484735c Compare May 19, 2024 07:14

xiangfu0 force-pushed the with-in-groupby branch from 484735c to 4671581 Compare May 19, 2024 13:02

xiangfu0 requested a review from Jackie-Jiang May 19, 2024 13:34

xiangfu0 force-pushed the with-in-groupby branch 5 times, most recently from 360739d to e5cd5ed Compare May 20, 2024 06:34

xiangfu0 requested review from vvivekiyer and walterddr May 20, 2024 06:39

xiangfu0 force-pushed the with-in-groupby branch from e5cd5ed to 3e923eb Compare May 20, 2024 07:30

gortiz reviewed May 20, 2024

View reviewed changes

...re/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ListAggFunction.java Outdated Show resolved Hide resolved

xiangfu0 force-pushed the with-in-groupby branch from 3e923eb to c5f45d4 Compare May 20, 2024 09:09

xiangfu0 requested a review from gortiz May 20, 2024 09:23

Jackie-Jiang reviewed May 21, 2024

View reviewed changes

xiangfu0 added 3 commits May 20, 2024 18:52

Support ListAgg WITHIN GROUP clause

d99e14a

fix rowCompartor schema

c51a7f4

Change rule to insert a sort for WithInGroup clause

85cb906

xiangfu0 force-pushed the with-in-groupby branch 4 times, most recently from 8bf72f0 to 9610e82 Compare May 21, 2024 03:22

Jackie-Jiang reviewed May 21, 2024

View reviewed changes

xiangfu0 force-pushed the with-in-groupby branch 2 times, most recently from 3c88a3c to a4d2cc5 Compare May 21, 2024 10:03

move isDistinct field out from FunctionContext

2a73f0c

xiangfu0 force-pushed the with-in-groupby branch from a4d2cc5 to 2a73f0c Compare May 21, 2024 10:12

xiangfu0 requested a review from Jackie-Jiang May 21, 2024 10:17

Jackie-Jiang approved these changes May 21, 2024

View reviewed changes

address comments

60d2a99

xiangfu0 force-pushed the with-in-groupby branch from 94a8676 to 60d2a99 Compare May 22, 2024 03:08

xiangfu0 merged commit 253ede9 into apache:master May 22, 2024

xiangfu0 deleted the with-in-groupby branch May 22, 2024 04:21

Jackie-Jiang mentioned this pull request Dec 4, 2024

add array_join for multi-valued column #10113

Closed

Jackie-Jiang added the documentation label Dec 4, 2024

Support ListAgg WITHIN GROUP clause #13146

Support ListAgg WITHIN GROUP clause #13146

Uh oh!

Conversation

xiangfu0 commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiangfu0 commented May 19, 2024

Uh oh!

xiangfu0 commented May 19, 2024

Uh oh!

xiangfu0 commented May 20, 2024

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiangfu0 commented May 21, 2024

Uh oh!

Jackie-Jiang May 21, 2024

Choose a reason for hiding this comment

Uh oh!

xiangfu0 May 21, 2024

Choose a reason for hiding this comment

Uh oh!

xiangfu0 May 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xiangfu0 commented May 13, 2024 •

edited

Loading

codecov-commenter commented May 13, 2024 •

edited

Loading