Adds support for leveraging StarTree index in conjunction with filtered aggregations #11886

egalpin · 2023-10-26T22:17:51Z

This PR aims to add support for leveraging StarTree index from filtered aggregations (i.e. SELECT COUNT(*) FILTER (WHERE foo='bar')), including filtered group-by aggregations. For queries with a mixture of star-tree-solvable aggregations and aggs which cannot be solved by star-tree, those aggs solvable via star-tree will do so iff all aggs in the same filter swimlane are solvable via star-tree (where swimlane means same FILTER WHERE expression, including lack of FILTER WHERE).

tags: [feature]

egalpin · 2023-10-26T22:22:00Z

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

-
-    // Use star-tree to solve the query if possible
-    List<StarTreeV2> starTrees = _indexSegment.getStarTrees();
-    if (starTrees != null && !_queryContext.isSkipStarTree()) {


Note: this PR would introduce a behaviour change (bugfix?) where AggregationPlanNode included an additional predicate to check if null handling is enabled before allowing use of StarTree index to solve the query. This PR consolidates this logic to one method within OperatorUtils.java, where the null handling predicate is kept such that both GroupByPlanNode and AggregationPlanNode will ensure null handling is disabled before allowing use of StarTree index.

if (starTrees != null && !_queryContext.isSkipStarTree() && !_queryContext.isNullHandlingEnabled()) {

egalpin · 2023-10-26T22:48:10Z

Marking as ready-for-review to get test results, but this should effectively be considered a draft at this point

codecov-commenter · 2023-10-26T23:14:00Z

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (41d1a12) 61.62% compared to head (10d1df9) 61.63%.
Report is 9 commits behind head on master.

Files	Patch %	Lines
.../org/apache/pinot/core/startree/StarTreeUtils.java	21.73%	17 Missing and 1 partial ⚠️
...t/core/operator/query/FilteredGroupByOperator.java	61.53%	3 Missing and 2 partials ⚠️
...ore/startree/executor/StarTreeGroupByExecutor.java	0.00%	5 Missing ⚠️
...re/operator/query/FilteredAggregationOperator.java	66.66%	2 Missing and 1 partial ⚠️
...aggregation/function/AggregationFunctionUtils.java	95.00%	1 Missing and 2 partials ⚠️
...che/pinot/core/operator/query/GroupByOperator.java	85.71%	0 Missing and 1 partial ⚠️
...rg/apache/pinot/core/plan/AggregationPlanNode.java	87.50%	0 Missing and 1 partial ⚠️
...ava/org/apache/pinot/core/plan/FilterPlanNode.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #11886   +/-   ##
=========================================
  Coverage     61.62%   61.63%           
- Complexity     1150     1152    +2     
=========================================
  Files          2385     2385           
  Lines        129275   129374   +99     
  Branches      20016    20030   +14     
=========================================
+ Hits          79668    79740   +72     
- Misses        43806    43821   +15     
- Partials       5801     5813   +12

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (ø)`
integration	`<0.01% <0.00%> (ø)`
integration1	`<0.01% <0.00%> (ø)`
integration2	`0.00% <0.00%> (ø)`
java-11	`61.59% <73.38%> (-0.01%)`	⬇️
java-21	`61.51% <73.38%> (+0.02%)`	⬆️
skip-bytebuffers-false	`61.62% <73.38%> (+<0.01%)`	⬆️
skip-bytebuffers-true	`61.47% <73.38%> (+0.02%)`	⬆️
temurin	`61.63% <73.38%> (+<0.01%)`	⬆️
unittests	`61.63% <73.38%> (+<0.01%)`	⬆️
unittests1	`46.98% <73.38%> (+<0.01%)`	⬆️
unittests2	`27.56% <0.00%> (-0.01%)`	⬇️

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.

…s cache working

…e even when nullhandling enabled to check integration tests

xiangfu0 · 2023-10-28T02:58:48Z

not sure if this is related: #11899

egalpin · 2023-10-28T03:35:50Z

@xiangfu0 yep! The goal of this PR would be to support exactly what’s described in #11899. Happy to collaborate, just let me know!

xiangfu0 · 2023-10-28T04:07:59Z

@xiangfu0 yep! The goal of this PR would be to support exactly what’s described in #11899. Happy to collaborate, just let me know!

Great!

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

… startree even when nullhandling enabled to check integration tests" This reverts commit 444dd5e.

egalpin · 2023-11-07T00:54:48Z

@xiangfu0 @Jackie-Jiang This can now be considered ready for review, as it's operationally functional. I'll add tests, but wanted to flag that this is ready for review in case that helps with velocity etc. See screen shots below for examples.

I noticed that this feature also provides an additional enhancement: being able to abuse FILTER aggs to allow queries to solve any agg which might be solvable via ST index even if one of the aggs in the query must be solved without the ST. This isn't currently possible; currently, all aggs must be solvable by the ST in order to proceed with its use.

Take for example the airlineStats example data. The following query would utilize ST:

select
  AirlineID,
  MAX(ArrDelay) as max_delay
FROM airlineStats
GROUP BY AirlineID

However, this would not (because MIN is not in the ST):

select
  AirlineID,
  MAX(ArrDelay) as max_delay,
  MIN(ArrDelay)
FROM airlineStats
GROUP BY AirlineID

After this change, one could rewrite the query like so:

select
  AirlineID,
  MAX(ArrDelay) FILTER (WHERE AirlineID != -1) AS max_delay,
  MIN(ArrDelay)
FROM airlineStats
GROUP BY AirlineID

This would allow the MAX to leverage the ST. (note that the FILTER WHERE could be placed on either aggregation, the key element is that adding a filter to the agg places the 2 aggs in different swimlanes which changes the decision making around whether or not ST can be used to answer the query)

egalpin · 2023-11-07T00:56:49Z

Contrived example to show the working of FILTER (only has a result for the one row that matches AirlineID). Note the 2 FILTER_STARTREE_INDEX in the plan.

…support-filtered-aggs

egalpin · 2023-11-08T05:00:35Z

@Jackie-Jiang @xiangfu0 this is now complete with tests and ready for review. Thanks!

Jackie-Jiang

Nice to see it fits to the current filtered aggregation architecture!

Jackie-Jiang · 2023-11-14T00:43:52Z

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

+import org.apache.pinot.core.operator.BaseProjectOperator;
+
+
+public interface ProjectionPlanNode extends PlanNode {


Suggest not adding this interface. We can do cast if necessary

Jackie-Jiang · 2023-11-14T00:44:41Z

.../src/main/java/org/apache/pinot/core/operator/filter/CombinedFilteredAggregationContext.java

+
+  public CombinedFilteredAggregationContext(BaseFilterOperator baseFilterOperator,
+      List<Pair<Predicate, PredicateEvaluator>> predicateEvaluators, @Nullable FilterContext mainFilterContext,
+      @Nonnull FilterContext subFilterContext, List<AggregationFunction> aggregationFunctions) {


(minor, convention) We don't usually use @Nonnull, but only annotate @Nullable and assume everything else as non-null

Jackie-Jiang · 2023-11-14T01:23:47Z

...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java

+    // Otherwise, there is no way to tell whether the 1st reload on server side is finished,
+    // which may hit the race condition that the 1st reload finishes after the 2nd reload is fully done.
+    // 10 seconds are still better than hitting race condition which will time out after 10 minutes.
+    Thread.sleep(10_000L);


We should avoid this kind of wait because it will waste resource on fast machine but flaky on slow machine. We don't need to add test in star-tree triggering test because the intention for this test is to just verify star-tree is created. We should add more tests into the StarTreeClusterIntegrationTest

Jackie-Jiang · 2023-11-14T01:24:04Z

...n-tests/src/test/java/org/apache/pinot/integration/tests/StarTreeClusterIntegrationTest.java

@@ -38,7 +38,9 @@
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;

+import static org.junit.Assert.assertFalse;


Wrong import

Jackie-Jiang · 2023-11-14T01:32:18Z

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

+    // Prevent instantiation, make checkstyle happy
+  }
+
+  public static ProjectionPlanNode maybeGetStartreeProjectionOperator(


I'd suggest moving this into the StarTreeUtils as

@Nullable public static BaseProjectOperator<?> createStarTreeBasedProjectOperator(...)

It returns null when star-tree cannot be used. This way we don't need to worry about using instanceof to identify whether it is star-tree based, and we don't need to create PlanNode out of BaseFilterOperator which should be avoided

In that case would you put the onus of creating the ProjectPlanNode on the caller of createStarTreeBasedProjectOperator if it returned null?

Yes. Note that I'm suggesting directly creating operator instead of plan node, so the caller is responsible for creating the BaseProjectOperator if star-tree cannot be used

Jackie-Jiang · 2023-11-14T01:37:38Z

.../src/main/java/org/apache/pinot/core/operator/filter/CombinedFilteredAggregationContext.java

+import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+
+
+public class CombinedFilteredAggregationContext {


This is used as a inner helper wrapper class for AggregationFunctionUtils.buildFilteredAggregateProjectOperators(). Suggest making it a private inner class, and we can directly access the fields within the class

… AggregationFunctionUtils

…support-filtered-aggs

egalpin · 2023-11-16T23:05:09Z

...-tests/src/test/java/org/apache/pinot/integration/tests/startree/StarTreeQueryGenerator.java

@@ -82,6 +83,16 @@ private String generateAggregation(String metricColumn) {
        metricColumn);
  }

+  private String generateFilteredAggregation(String metricColumn) {


@Jackie-Jiang these tests seem to consistently fail when run using the multi-stage engine. Are filtered aggregations supported in multi-stage, or not yet?

attempted fix in 990bf5a

egalpin · 2023-11-18T18:56:00Z

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

@@ -214,46 +214,69 @@ public static Object getConvertedFinalResult(DataTable dataTable, ColumnDataType
    }
  }



Much better, I was not liking the nested Pair usage haha. Thanks!

egalpin commented Oct 26, 2023

View reviewed changes

egalpin marked this pull request as ready for review October 26, 2023 22:47

egalpin added 9 commits October 26, 2023 16:53

Adds OperatorUtils to re-use logic of StarTree feasibility

a417d98

WIP

f8205af

WIP - combining predicate evaluators

5c05a38

WIP - no errors in AggregationFunctionUtils.java, predicate evaluator…

a0af786

…s cache working

WIP - possibly functional filteredGroupBy w/ ST

2cb208b

WIP - possibly functional FilteredAggOp w/ ST

83ebf15

WIP - removes some dev artifacts

715578f

Adds missing license to new file

b2b35e0

Fixes checkstyle errors

e873508

egalpin force-pushed the egalpin/startree-support-filtered-aggs branch from 90d91fe to e873508 Compare October 27, 2023 16:16

DELETE ME - intentionally reverts to bug in groupbyplan using startre…

444dd5e

…e even when nullhandling enabled to check integration tests

xiangfu0 added the feature label Oct 28, 2023

egalpin commented Oct 28, 2023

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/operator/query/OperatorUtils.java Outdated Show resolved Hide resolved

egalpin added 6 commits October 30, 2023 13:35

Revert "DELETE ME - intentionally reverts to bug in groupbyplan using…

5a0def6

… startree even when nullhandling enabled to check integration tests" This reverts commit 444dd5e.

Adds nullHandling support to CombinedFilterOperator

9dcad72

Checkpoint - Adds FilteredAggregationContext

d5d40c0

Fixes checkStyle violations

c034ce3

WIP - Adds ProjectionPlanNode

5b65ce7

WIP - Passes around Boolean with BaseProjectOperator

fa62992

Jackie-Jiang added the enhancement label Nov 3, 2023

egalpin added 3 commits November 6, 2023 10:57

Applies spotless linter changes

831db53

DELETE ME - local dev setup

dbb411f

Milestone: functional filtered group-by agg with ST

1943b37

egalpin added 5 commits November 7, 2023 10:18

Empty commit to re-run tests

d2aab67

Adds startree integration tests for filtered agg

3879dc9

Merge remote-tracking branch 'upstream/master' into egalpin/startree-…

80e89b9

…support-filtered-aggs

Uses new static FilterContext producer

768c4ce

Brings ST integration test up-to-date with latest constructor

ef2cff0

egalpin added 2 commits November 8, 2023 08:51

Only verify plan for hard-coded queries

e4bcde5

Clears ST index config after ST tests

6ecd0a4

Jackie-Jiang reviewed Nov 14, 2023

View reviewed changes

egalpin added 10 commits November 14, 2023 15:53

Moves CombinedFilteredAggregationContext to be static nested class of…

e5bb66e

… AggregationFunctionUtils

Removes use of @nonnull annotation

1a75079

Removes OperatorUtils and ProjectionPlanNode

8d94b51

Extracts projectOperator construction to AggFuncUtils method

e9f8bb8

Removes filtered ST agg test from OfflineClusterIntegrationTest

259b833

Uses testng assertFalse

13aadf1

Spotless linting changes

7678323

Always set _filter for FilterPlanNode

ba13c36

Adds generated filtered agg ST tests

a1931cc

Merge remote-tracking branch 'upstream/master' into egalpin/startree-…

ce3b7f3

…support-filtered-aggs

egalpin commented Nov 16, 2023

View reviewed changes

egalpin and others added 3 commits November 16, 2023 16:08

Handles the case of CONSTANT filter in FilterPlanNode

990bf5a

no-op to check for flaky test

0700839

Cleanup and introduce AggregationInfo

10d1df9

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

egalpin commented Nov 18, 2023

View reviewed changes

Jackie-Jiang approved these changes Nov 18, 2023

View reviewed changes

Jackie-Jiang merged commit f7f8260 into apache:master Nov 18, 2023
19 checks passed

egalpin deleted the egalpin/startree-support-filtered-aggs branch November 19, 2023 05:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for leveraging StarTree index in conjunction with filtered aggregations #11886

Adds support for leveraging StarTree index in conjunction with filtered aggregations #11886

egalpin commented Oct 26, 2023 •

edited

Loading

egalpin Oct 26, 2023

egalpin commented Oct 26, 2023

codecov-commenter commented Oct 26, 2023 •

edited

Loading

xiangfu0 commented Oct 28, 2023

egalpin commented Oct 28, 2023

xiangfu0 commented Oct 28, 2023

egalpin commented Nov 7, 2023 •

edited

Loading

egalpin commented Nov 7, 2023 •

edited

Loading

egalpin commented Nov 8, 2023

Jackie-Jiang left a comment

Jackie-Jiang Nov 14, 2023

Jackie-Jiang Nov 14, 2023

Jackie-Jiang Nov 14, 2023

Jackie-Jiang Nov 14, 2023

Jackie-Jiang Nov 14, 2023

egalpin Nov 15, 2023

Jackie-Jiang Nov 16, 2023

Jackie-Jiang Nov 14, 2023

egalpin Nov 16, 2023

egalpin Nov 17, 2023

egalpin Nov 18, 2023

		import org.apache.pinot.core.operator.BaseProjectOperator;


		public interface ProjectionPlanNode extends PlanNode {

		import org.apache.pinot.core.query.aggregation.function.AggregationFunction;


		public class CombinedFilteredAggregationContext {

		@@ -214,46 +214,69 @@ public static Object getConvertedFinalResult(DataTable dataTable, ColumnDataType
		}
		}

Adds support for leveraging StarTree index in conjunction with filtered aggregations #11886

Adds support for leveraging StarTree index in conjunction with filtered aggregations #11886

Conversation

egalpin commented Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

egalpin commented Oct 26, 2023

codecov-commenter commented Oct 26, 2023 • edited Loading

Codecov Report

xiangfu0 commented Oct 28, 2023

egalpin commented Oct 28, 2023

xiangfu0 commented Oct 28, 2023

egalpin commented Nov 7, 2023 • edited Loading

egalpin commented Nov 7, 2023 • edited Loading

egalpin commented Nov 8, 2023

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

egalpin commented Oct 26, 2023 •

edited

Loading

codecov-commenter commented Oct 26, 2023 •

edited

Loading

egalpin commented Nov 7, 2023 •

edited

Loading

egalpin commented Nov 7, 2023 •

edited

Loading