Proper null handling in Aggregation functions for SV data types #9086

nizarhejazi · 2022-07-21T17:27:57Z

Proper null handling for SV data types in the following aggregation functions:

AVG
MIN
MAX
COUNT
SUM
SUMPRECISION

Added many unit tests (for different data types) for diff. aggregation functions.

feature, release-notes

…rs / matchers

codecov-commenter · 2022-07-21T23:29:27Z

Codecov Report

Merging #9086 (20cc8f8) into master (561e471) will increase coverage by 0.29%.
The diff coverage is 62.16%.

@@             Coverage Diff              @@
##             master    #9086      +/-   ##
============================================
+ Coverage     68.39%   68.68%   +0.29%     
+ Complexity     4757     4672      -85     
============================================
  Files          1848     1848              
  Lines         97900    98386     +486     
  Branches      14746    14932     +186     
============================================
+ Hits          66960    67579     +619     
+ Misses        26251    26008     -243     
- Partials       4689     4799     +110

Flag	Coverage Δ
integration1	`26.38% <11.17%> (?)`
integration2	`?`
unittests1	`66.97% <61.62%> (+0.03%)`	⬆️
unittests2	`15.30% <0.36%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
...a/org/apache/pinot/core/plan/DistinctPlanNode.java	`86.95% <0.00%> (ø)`
...tion/function/SumPrecisionAggregationFunction.java	`34.26% <39.83%> (+0.43%)`	⬆️
...aggregation/groupby/ObjectGroupByResultHolder.java	`87.50% <50.00%> (+1.13%)`	⬆️
...aggregation/function/CountAggregationFunction.java	`71.25% <57.57%> (+1.25%)`	⬆️
...y/aggregation/function/AvgAggregationFunction.java	`81.67% <60.00%> (-15.66%)`	⬇️
...y/aggregation/function/MinAggregationFunction.java	`71.32% <62.35%> (-15.56%)`	⬇️
...y/aggregation/function/MaxAggregationFunction.java	`72.53% <64.28%> (-14.36%)`	⬇️
...y/aggregation/function/SumAggregationFunction.java	`73.17% <66.66%> (-10.83%)`	⬇️
...core/query/reduce/AggregationDataTableReducer.java	`80.95% <68.18%> (-5.33%)`	⬇️
...rg/apache/pinot/core/plan/AggregationPlanNode.java	`88.99% <87.50%> (+0.20%)`	⬆️
... and 254 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Jackie-Jiang

Suggest breaking it into 2 parts, one for the filter and one for the aggregation

Jackie-Jiang · 2022-07-23T03:48:04Z

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

      for (Expression operand : operands) {
        arguments.add(getExpression(operand));
      }
+      if (arguments.size() == 0 && functionName.equalsIgnoreCase(AggregationFunctionType.COUNT.getName())) {


Arguments should never be empty

Count(*) has no operands. Since I allow now count(colName), I removed the if in lines 84-88 and add this if here instead.

count(*) should have a single identifier operand with name "*"

yes, it is never empty except in one test: testSqlWithFinalRowCountChecker (part of pinot-query-runtime tests).

Basically, pinot-query-runtime causes this issue and send count() without any argument.

@walterddr I think this is a bug in the multi-stage engine where the argument is not passed for COUNT. Can you please take a look?

ok yeah so that pinot-server expects a * on count. got it I can make the fix.

pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/IntermediateResultsBlock.java

pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/SVScanDocIdIterator.java

...-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java

...t-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/PredicateEvaluator.java

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

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

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

…null_agg

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

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

Jackie-Jiang · 2022-07-30T19:07:48Z

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

    if (blockValSetMap.isEmpty()) {
      aggregationResultHolder.setValue(aggregationResultHolder.getDoubleResult() + length);
+    } else if (!blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION)) {
+      if (!_nullHandlingEnabled) {


Could this branch be hit?

Suggest changing the branching logic as following for clarity:

if (blockValSetMap.isEmpty()) { ... } else if (_nullHandlingEnabled) { ... } else { // Star-tree pre-aggregated values ... }

I still need to check for: !blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION)

Why is that? When null handling is enabled, we cannot use star-tree, so we shouldn't need this check right? Basically when the blockValSetMap is empty, either null handling is enabled or star-tree is used, and they should be mutual exclusive

@Jackie-Jiang We have three branches:
1- blockValSetMap.isEmpty() for count(*), and no StarTree.
2- !blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION) for count(columnName), and no StarTree (please see the ! at the beginning). Example: count(bigDecimalColumn). Note: we can have count(bigDecimalColumn) and nullHandling is either enable or not enabled.
3- StarTree.

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

pinot-core/src/main/java/org/apache/pinot/core/query/selection/SelectionOperatorUtils.java

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

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

pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkScanDocIdIterators.java

...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java

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

Jackie-Jiang

Almost ready to be merged

Jackie-Jiang · 2022-08-02T00:57:56Z

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

    if (blockValSetMap.isEmpty()) {
      aggregationResultHolder.setValue(aggregationResultHolder.getDoubleResult() + length);
+    } else if (!blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION)) {
+      if (!_nullHandlingEnabled) {


Why is that? When null handling is enabled, we cannot use star-tree, so we shouldn't need this check right? Basically when the blockValSetMap is empty, either null handling is enabled or star-tree is used, and they should be mutual exclusive

Jackie-Jiang · 2022-08-02T00:59:49Z

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

  public void aggregateGroupBySV(int length, int[] groupKeyArray, GroupByResultHolder groupByResultHolder,
      Map<ExpressionContext, BlockValSet> blockValSetMap) {
-    if (blockValSetMap.isEmpty()) {
+    if (blockValSetMap.isEmpty() || !blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION)) {


Similar here, we should be able to simplify it into 3 conditions:

no value set

null handling enabled

star-tree

@Jackie-Jiang We have three branches:
1- blockValSetMap.isEmpty() for count(*), and no StarTree.
2- !blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION) for count(columnName), and no StarTree (please see the ! at the beginning). Example: count(bigDecimalColumn). Note: we can have count(bigDecimalColumn) and nullHandling is either enable or not enabled.
3- StarTree.

When both null handling and star-tree are not enabled, will it pass in non-empty blockValSetMap? Ideally it should pass in empty blockValSetMap, or it could add extra overhead for existing use cases

I think I find the reason. When null handling is not enabled, ideally getInputExpressions() should return empty list to preserve the existing behavior, which is changed because we changed it to extend BaseSingleInputAggregationFunction. Since most methods in BaseSingleInputAggregationFunction cannot be reused, suggest not extending it, and keep the existing behavior when null handling is not enabled

Applied a commit to demonstrate the idea: b684534
Let's see if it can pass all the tests

The overhead is should be minimal. I don't have strong opinion either way. Will apply your commit.

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

...n/java/org/apache/pinot/core/query/aggregation/function/SumPrecisionAggregationFunction.java

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

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

...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java

…null_agg

...n/java/org/apache/pinot/core/query/aggregation/function/SumPrecisionAggregationFunction.java

Jackie-Jiang · 2022-08-03T18:50:22Z

...n/java/org/apache/pinot/core/query/aggregation/function/SumPrecisionAggregationFunction.java

+          for (int i = 0; i < length; i++) {
+            if (!nullBitmap.contains(i)) {
+              // TODO(nhejazi): throw an exception here instead of ignoring infinite values?
+              if (Double.isFinite(doubleValues[i])) {


(minor) Suggest not having this check to keep the behavior consistent

fyi: this is gonna fail the whole SumPrecisionAgg function. So the previous behaviour is just throwing an exception and the operation failing.
@Jackie-Jiang Do you wanna still remove this check? I

Currently the behavior is inconsistent without null handling (throw exception) and with null handling (values ignored). We may revisit this since you already added a TODO here

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

Jackie-Jiang

LGTM! This is a very important feature. Thanks for contributing it!

Jackie-Jiang · 2022-08-10T18:41:15Z

Related to #8697

nizarhejazi added 4 commits July 21, 2022 09:52

Proper null handling in Aggregation functions, and comparison operato…

c4ca1ec

…rs / matchers

Merge branch 'master' into null_agg

2c78221

Merge branch 'master' into null_agg

c776c4d

address checkstyle errors

6deba9d

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Jul 21, 2022

Update AggregationOperator in pinot-query-runtime

30564b1

nizarhejazi and others added 2 commits July 21, 2022 17:08

update the query results of count agg functions

7757dac

Merge branch 'apache:master' into null_agg

6f7a65d

Jackie-Jiang reviewed Jul 23, 2022

View reviewed changes

nizarhejazi added 5 commits July 25, 2022 14:48

address comments

461c3d4

Merge branch 'master' into null_agg

22fc76f

Merge branch 'null_agg' of https://github.com/nizarhejazi/pinot into …

6d739a5

…null_agg

cleanup

e1e4c9f

remove null handling in membership, equality and inequality operators

5fefc20

nizarhejazi changed the title ~~Proper null handling in Aggregation functions, and comparison operators / matchers for SV data types~~ Proper null handling in Aggregation functions for SV data types Jul 25, 2022

nizarhejazi added 11 commits July 25, 2022 15:22

address checkstyle warning

d80f53c

one more cleanup

180f955

address comments

992f5b2

Merge branch 'master' into null_agg

8d13d4c

address null handling in count aggregate w/ group-by

2950cfd

Merge branch 'master' into null_agg

2b8a1e5

address checkstyle warnings

9a586a2

update SumPrecision

d9f5972

fix a test

5a13bcc

address one comment

a25d59b

Merge branch 'master' into null_agg

793a6fb

Jackie-Jiang reviewed Jul 30, 2022

View reviewed changes

walterddr reviewed Jul 31, 2022

View reviewed changes

pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java Show resolved Hide resolved

nizarhejazi added 5 commits July 31, 2022 15:48

address comments

550b837

address comments

b48dc92

update if code for readability

5b4b927

revert a change

5f0c786

Merge branch 'master' into null_agg

c50fe5a

Jackie-Jiang reviewed Aug 2, 2022

View reviewed changes

Jackie-Jiang and others added 4 commits August 2, 2022 15:42

Fix CountAggregationFunction

b684534

address comments

f188e82

Merge branch 'master' into null_agg

2d7125e

Merge branch 'null_agg' of https://github.com/nizarhejazi/pinot into …

20cc8f8

…null_agg

Jackie-Jiang reviewed Aug 3, 2022

View reviewed changes

address comments

3c279b7

Jackie-Jiang approved these changes Aug 3, 2022

View reviewed changes

Jackie-Jiang merged commit d617f3b into apache:master Aug 3, 2022

yashmayya mentioned this pull request Sep 24, 2024

Add null handling support for aggregations grouped by MV columns #14071

Merged

Proper null handling in Aggregation functions for SV data types #9086

Proper null handling in Aggregation functions for SV data types #9086

Uh oh!

Conversation

nizarhejazi commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 21, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

nizarhejazi Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizarhejazi Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nizarhejazi commented Jul 21, 2022 •

edited

Loading

codecov-commenter commented Jul 21, 2022 •

edited

Loading

nizarhejazi Jul 25, 2022 •

edited

Loading

nizarhejazi Jul 29, 2022 •

edited

Loading

nizarhejazi Aug 3, 2022 •

edited

Loading