[Issue 7519] Adds support for multiple filtered/unfiltered aggregations with GROUP BY #10000

egalpin · 2022-12-17T01:25:22Z

Following @atris's awesome work[1] to support FILTER expressions[2][3], this PR aims to introduce support for FILTER expressions simultaneously with GROUP BY. The same swim-lane pattern established in the initial FILTER expression PR[1] is re-used/shared here by groupBy processing.

Fixes #7519

[1] #7916
[2] #7519
[3] https://docs.google.com/document/d/1ZM-2c0jJkbeJ61m8sJF0qj19t5UYLhnTFvIAz-HCJmk

egalpin · 2022-12-17T01:33:47Z

Will work on the checkstyle violations. Has there ever been consideration of using Spotless[1] which can both check as well as autofix linting issues? I think pinot-style specifically is not supported, but perhaps support could be added? Auto-fixing would be a huge win.

[1] https://github.com/diffplug/spotless

…Operator

codecov-commenter · 2022-12-19T18:23:21Z

Codecov Report

Merging #10000 (751dc18) into master (d3ea8dc) will increase coverage by 54.53%.
The diff coverage is 82.87%.

@@              Coverage Diff              @@
##             master   #10000       +/-   ##
=============================================
+ Coverage     15.86%   70.40%   +54.53%     
- Complexity      176     5693     +5517     
=============================================
  Files          1931     1996       +65     
  Lines        104306   107719     +3413     
  Branches      15901    16376      +475     
=============================================
+ Hits          16551    75842    +59291     
+ Misses        86531    26577    -59954     
- Partials       1224     5300     +4076

Flag	Coverage Δ
integration1	`25.10% <4.97%> (?)`
integration2	`24.17% <4.97%> (?)`
unittests1	`67.91% <82.87%> (?)`
unittests2	`13.55% <0.00%> (-2.32%)`	⬇️

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

Impacted Files	Coverage Δ
.../apache/pinot/common/exception/QueryException.java	`94.44% <ø> (+94.44%)`	⬆️
...pinot/core/query/request/context/QueryContext.java	`99.06% <ø> (+99.06%)`	⬆️
...va/org/apache/pinot/query/runtime/QueryRunner.java	`84.26% <ø> (+0.17%)`	⬆️
...t/core/operator/query/FilteredGroupByOperator.java	`70.00% <70.00%> (ø)`
...inot/segment/local/customobject/VarianceTuple.java	`64.28% <75.00%> (+64.28%)`	⬆️
...aggregation/function/AggregationFunctionUtils.java	`76.66% <86.04%> (+76.66%)`	⬆️
.../apache/pinot/common/datablock/DataBlockUtils.java	`90.43% <100.00%> (+90.43%)`	⬆️
...rg/apache/pinot/core/plan/AggregationPlanNode.java	`90.78% <100.00%> (+90.78%)`	⬆️
...va/org/apache/pinot/core/plan/GroupByPlanNode.java	`89.58% <100.00%> (+89.58%)`	⬆️
...ry/aggregation/groupby/DefaultGroupByExecutor.java	`96.92% <100.00%> (+96.92%)`	⬆️
... and 1542 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

egalpin · 2022-12-19T23:35:09Z

@Jackie-Jiang this is ready for review 😊

Jackie-Jiang

Very well done! Mostly minor comments

Jackie-Jiang · 2023-01-03T19:48:18Z

pinot-core/src/main/java/org/apache/pinot/core/operator/query/FilteredGroupByOperator.java

+  private GroupKeyGenerator _groupKeyGenerator = null;
+
+  public FilteredGroupByOperator(
+      @Nullable AggregationFunction[] aggregationFunctions,


I don't think it can be null. Actually if it is null, line 83 will throw NPE

Jackie-Jiang · 2023-01-03T19:54:23Z

pinot-core/src/main/java/org/apache/pinot/core/operator/query/FilteredGroupByOperator.java

+  private TableResizer _tableResizer;
+  private GroupKeyGenerator _groupKeyGenerator = null;


(minor) These 2 member variables can be converted to local

Jackie-Jiang · 2023-01-03T19:59:05Z

pinot-core/src/main/java/org/apache/pinot/core/operator/query/FilteredGroupByOperator.java

+
+      // Perform aggregation group-by on all the blocks
+      DefaultGroupByExecutor groupByExecutor;
+      if (_groupKeyGenerator == null) {


This is very smart to share the same group-key generator. Let's add some comments about why we need to do so

Jackie-Jiang · 2023-01-03T20:00:45Z

pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/groupby/GroupByTrimTest.java

@@ -130,7 +129,7 @@ void testGroupByTrim(QueryContext queryContext, int minSegmentGroupTrimSize, int
    // Extract the execution result
    List<Pair<Double, Double>> extractedResult = extractTestResult(resultsBlock.getTable());

-    assertEquals(extractedResult, expectedResult);
+    Assert.assertEquals(extractedResult, expectedResult);


(nit) We usually use static import for Assert in test

I had changed this due to checkstyle violations:

[WARNING] (imports) AvoidStaticImport: Using a static member import should be avoided

I can revert back so long as checkstyle still passes

Jackie-Jiang · 2023-01-03T20:02:05Z

pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java

+    String nonFilterQuery =
+        "SELECT SUM(INT_COL), SUM(CASE WHEN INT_COL > 25000 THEN INT_COL ELSE 0 END) AS total_sum FROM MyTable GROUP "
+            + "BY INT_COL";
+    testQuery(filterQuery, nonFilterQuery);


Let's add some more tests to guarantee that it works as expected

Jackie-Jiang · 2023-01-03T20:05:01Z

pinot-core/src/main/java/org/apache/pinot/core/plan/GroupByPlanNode.java

+    return nonFilteredGroupByPlan();
+  }
+
+  private FilteredGroupByOperator filteredGroupByPlan() {


(minor) Rename to buildFilteredGroupByPlan() and buildNonFilteredGroupByPlan()

Jackie-Jiang · 2023-01-03T20:06:00Z

...src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java

-  public static Set<ExpressionContext> collectExpressionsToTransform(AggregationFunction[] aggregationFunctions,
-      @Nullable ExpressionContext[] groupByExpressions) {
+  public static Set<ExpressionContext> collectExpressionsToTransform(
+      @Nullable AggregationFunction[] aggregationFunctions, @Nullable ExpressionContext[] groupByExpressions) {


aggregationFunctions should always be non-null

Jackie-Jiang · 2023-01-03T20:14:31Z

...re/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java

+  protected GroupKeyGenerator _groupKeyGenerator;
+  protected GroupByResultHolder[] _groupByResultHolders;


These 2 variables can still be final

Jackie-Jiang · 2023-01-03T20:34:31Z

...re/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java

+
+  public DefaultGroupByExecutor(QueryContext queryContext, ExpressionContext[] groupByExpressions,
+      TransformOperator transformOperator) {
+


(nit) Remove empty line

Jackie-Jiang · 2023-01-03T20:36:51Z

...re/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java

-    _aggregationFunctions = queryContext.getAggregationFunctions();
+  public DefaultGroupByExecutor(QueryContext queryContext, AggregationFunction[] aggregationFunctions,
+      ExpressionContext[] groupByExpressions, TransformOperator transformOperator,
+      GroupKeyGenerator groupKeyGenerator) {


Annotate groupKeyGenerator as Nullable

egalpin · 2023-01-03T21:40:41Z

pinot-core/src/main/java/org/apache/pinot/core/operator/query/FilteredGroupByOperator.java

+      // Perform aggregation group-by on all the blocks
+      DefaultGroupByExecutor groupByExecutor;
+      if (groupKeyGenerator == null) {
+        // The group key generator should be shared across all AggregationFunctions so that agg results can be


@Jackie-Jiang please let me know where I may have misstated the reasoning

egalpin · 2023-01-03T23:33:42Z

@Jackie-Jiang added more tests in aeebef0, let me know if there are other specific cases to cover.

This PR is ready for re-review

egalpin · 2023-01-03T23:35:10Z

Oops, somehow clicking request re-review from Jackie also removed Mayank? Removal was unintentional

egalpin · 2023-01-04T20:00:00Z

@Jackie-Jiang the integration test failure looks unrelated to me as the same tests are failing in the same way on other PRs / the failures don't seem to point to the changed areas of code. Can you confirm?

Jackie-Jiang

LGTM!

kishoreg · 2023-01-05T03:06:27Z

Thanks @egalpin for your contribution. Please add docs when you get a chance if its not done already

egalpin · 2023-01-05T16:38:51Z

Will do @kishoreg 👍

ankitsultana · 2023-09-26T20:01:21Z

@egalpin : can you update documentation as well? https://docs.pinot.apache.org/users/user-guide-query/query-syntax/supported-aggregations#filter-clause-in-aggregation

This still says the following:

NOTE: The FILTER clause is currently supported for aggregation-only queries, i.e., GROUP BY
is not supported.

egalpin · 2023-10-13T22:14:16Z

Thanks! pinot-contrib/pinot-docs#250

egalpin added 2 commits December 16, 2022 17:47

Working multiple filtered/unfiltered GROUP BY

25fc5d9

Fixes lint issues

67adb07

egalpin force-pushed the egalpin/filter-with-groupby branch from db20331 to 67adb07 Compare December 17, 2022 01:47

egalpin added 2 commits December 19, 2022 09:27

Removes development TODOs

4762f9c

Fixes GroupByTrimTest by using Operator interface rather than GroupBy…

0327312

…Operator

egalpin force-pushed the egalpin/filter-with-groupby branch from a7e6143 to 0327312 Compare December 19, 2022 17:44

Removes commented out code dismissing FILTER with GROUP BY

4e7f31a

Fixes/adds test for FILTER + GROUP BY

cbff027

egalpin changed the title ~~[Issue 7519] Adds support for multiple filtered/unfiltered with GROUP BY~~ [Issue 7519] Adds support for multiple filtered/unfiltered aggregate with GROUP BY Dec 20, 2022

egalpin changed the title ~~[Issue 7519] Adds support for multiple filtered/unfiltered aggregate with GROUP BY~~ [Issue 7519] Adds support for multiple filtered/unfiltered aggregations with GROUP BY Dec 20, 2022

xiangfu0 requested review from Jackie-Jiang and mayankshriv December 21, 2022 23:53

Jackie-Jiang added the feature label Dec 25, 2022

Jackie-Jiang reviewed Jan 3, 2023

View reviewed changes

egalpin added 5 commits January 3, 2023 13:13

Converts _tableResizer and _groupKeyGenerator to local vars

c6aa297

Adds comment string RE sharing GroupKeyGenerator

24e010e

Ensures _groupKeyGenerator and _groupByResultHolders are final

1dee807

Renames (non)filteredGroupByPlan -> build(Non)FilteredGroupByPlan

602cdbe

Removes null check around agg functions for filter aggs

429cd41

egalpin commented Jan 3, 2023

View reviewed changes

egalpin added 2 commits January 3, 2023 15:27

Adds more GROUP BY tests for filtered aggs

aeebef0

Fixes checkstyle violations

24d365e

egalpin requested review from Jackie-Jiang and removed request for mayankshriv January 3, 2023 23:33

richardstartin mentioned this pull request Jan 4, 2023

push utf8 comparison down into ValueReader #10044

Merged

Minor reformat

751dc18

Jackie-Jiang approved these changes Jan 5, 2023

View reviewed changes

Jackie-Jiang merged commit 303b1a7 into apache:master Jan 5, 2023

Jackie-Jiang added the release-notes Referenced by PRs that need attention when compiling the next release notes label Jan 5, 2023

egalpin mentioned this pull request Jan 10, 2023

Fixes filtered agg result column naming and filtered agg order-by compat #10092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 7519] Adds support for multiple filtered/unfiltered aggregations with GROUP BY #10000

[Issue 7519] Adds support for multiple filtered/unfiltered aggregations with GROUP BY #10000

egalpin commented Dec 17, 2022 •

edited

Loading

egalpin commented Dec 17, 2022

codecov-commenter commented Dec 19, 2022 •

edited

Loading

egalpin commented Dec 19, 2022

Jackie-Jiang left a comment

Jackie-Jiang Jan 3, 2023

Jackie-Jiang Jan 3, 2023

Jackie-Jiang Jan 3, 2023

Jackie-Jiang Jan 3, 2023

egalpin Jan 3, 2023

Jackie-Jiang Jan 3, 2023

Jackie-Jiang Jan 3, 2023

Jackie-Jiang Jan 3, 2023

Jackie-Jiang Jan 3, 2023

Jackie-Jiang Jan 3, 2023

Jackie-Jiang Jan 3, 2023

egalpin Jan 3, 2023

egalpin commented Jan 3, 2023

egalpin commented Jan 3, 2023

egalpin commented Jan 4, 2023

Jackie-Jiang left a comment

kishoreg commented Jan 5, 2023

egalpin commented Jan 5, 2023

ankitsultana commented Sep 26, 2023

egalpin commented Oct 13, 2023

		private TableResizer _tableResizer;
		private GroupKeyGenerator _groupKeyGenerator = null;

		protected GroupKeyGenerator _groupKeyGenerator;
		protected GroupByResultHolder[] _groupByResultHolders;


		public DefaultGroupByExecutor(QueryContext queryContext, ExpressionContext[] groupByExpressions,
		TransformOperator transformOperator) {

[Issue 7519] Adds support for multiple filtered/unfiltered aggregations with GROUP BY #10000

[Issue 7519] Adds support for multiple filtered/unfiltered aggregations with GROUP BY #10000

Conversation

egalpin commented Dec 17, 2022 • edited Loading

egalpin commented Dec 17, 2022

codecov-commenter commented Dec 19, 2022 • edited Loading

Codecov Report

egalpin commented Dec 19, 2022

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egalpin commented Jan 3, 2023

egalpin commented Jan 3, 2023

egalpin commented Jan 4, 2023

Jackie-Jiang left a comment

Choose a reason for hiding this comment

kishoreg commented Jan 5, 2023

egalpin commented Jan 5, 2023

ankitsultana commented Sep 26, 2023

egalpin commented Oct 13, 2023

egalpin commented Dec 17, 2022 •

edited

Loading

codecov-commenter commented Dec 19, 2022 •

edited

Loading