Proper null handling in SELECT, ORDER BY, DISTINCT, and GROUP BY by nizarhejazi · Pull Request #8927 · apache/pinot

nizarhejazi · 2022-06-19T23:48:18Z

Proper NULL handling in:

SELECT with and without ORDER BY
DISTINCT
GROUP BY
DISTINCT

For Single-valued columns of all data types with:

Raw encoding
Dictionary encoding

Unit tests:

Updated BigDecimalQueriesTest to work with null values.
Added BooleanNullEnabledNoDictQueriesTest.
Added AllNullQueriesTest where all values are null.
Added NullEnabledQueriesTest where some values are nulls.

codecov-commenter · 2022-06-20T01:03:17Z

Codecov Report

Merging #8927 (b661f25) into master (84478b6) will decrease coverage by 6.44%.
The diff coverage is 79.72%.

❗ Current head b661f25 differs from pull request most recent head 85e806a. Consider uploading reports for the commit 85e806a to get more accurate results

@@             Coverage Diff              @@
##             master    #8927      +/-   ##
============================================
- Coverage     70.13%   63.68%   -6.45%     
+ Complexity     4741     4719      -22     
============================================
  Files          1831     1784      -47     
  Lines         96382    94815    -1567     
  Branches      14408    14371      -37     
============================================
- Hits          67594    60381    -7213     
- Misses        24125    30101    +5976     
+ Partials       4663     4333     -330

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests1	`66.96% <79.72%> (+0.11%)`	⬆️
unittests2	`15.36% <0.00%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
.../java/org/apache/pinot/common/utils/DataTable.java	`95.45% <ø> (ø)`
...che/pinot/core/common/datablock/BaseDataBlock.java	`77.41% <0.00%> (-0.28%)`	⬇️
...che/pinot/core/common/datatable/BaseDataTable.java	`78.94% <ø> (ø)`
...reaming/StreamingSelectionOnlyCombineOperator.java	`0.00% <0.00%> (-65.31%)`	⬇️
...ator/streaming/StreamingSelectionOnlyOperator.java	`0.00% <0.00%> (-90.00%)`	⬇️
...ctionaryBasedSingleColumnDistinctOnlyExecutor.java	`0.00% <0.00%> (ø)`
...e/pinot/core/query/reduce/RowBasedBlockValSet.java	`15.94% <0.00%> (-0.24%)`	⬇️
...main/java/org/apache/pinot/spi/data/FieldSpec.java	`82.03% <ø> (ø)`
...va/org/apache/pinot/spi/utils/CommonConstants.java	`28.57% <ø> (ø)`
.../raw/RawBytesSingleColumnDistinctOnlyExecutor.java	`42.10% <28.57%> (-37.90%)`	⬇️
... and 480 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84478b6...85e806a. Read the comment docs.

Jackie-Jiang

In high level, we should not decide whether to enable query null handling based on the whether there is null value vector. This flag should be passed from the broker as a query option, and we can introduce another table config to enable it. This can ensure all the segments are executed using the same mode.

pinot-common/src/main/java/org/apache/pinot/common/utils/DataTable.java

pinot-core/src/main/java/org/apache/pinot/core/common/RowBasedBlockValueFetcher.java

pinot-core/src/main/java/org/apache/pinot/core/common/datablock/RowDataBlock.java

pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java

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

...core/src/main/java/org/apache/pinot/core/operator/combine/GroupByOrderByCombineOperator.java

pinot-common/src/thrift/query.thrift

pinot-core/src/main/java/org/apache/pinot/core/common/datablock/RowDataBlock.java

pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java

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

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

...ain/java/org/apache/pinot/core/operator/streaming/StreamingSelectionOnlyCombineOperator.java

pinot-core/src/main/java/org/apache/pinot/core/operator/transform/TransformOperator.java

.../org/apache/pinot/core/operator/combine/MinMaxValueBasedSelectionOrderByCombineOperator.java

Jackie-Jiang · 2022-06-30T14:10:32Z

pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/ProjectionBlockValSet.java

    _dataBlockCache = dataBlockCache;
    _column = column;
    _dataSource = dataSource;
+    NullValueVectorReader nullValueReader = _dataSource == null ? null : _dataSource.getNullValueVector();


Data source should never be null. The computation should be lazy, and the operator should decide whether to read it based on if null handling is enabled

Updated to do computation lazily, and removed null check.

Moved the logic to getNullBitmap() method. Please note that this method is called only when nullHandlingEnabled is set to true.

Jackie-Jiang

There are plenty of comments, but the PR is in good shape overall. Good job!

pinot-core/src/main/java/org/apache/pinot/core/common/datablock/RowDataBlock.java

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java

pinot-core/src/main/java/org/apache/pinot/core/util/QueryOptionsUtils.java

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

...a/org/apache/pinot/core/query/distinct/raw/RawStringSingleColumnDistinctOrderByExecutor.java

...java/org/apache/pinot/core/query/distinct/raw/RawStringSingleColumnDistinctOnlyExecutor.java

pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java

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

Jackie-Jiang

Mostly good.
cc @siddharthteotia to also take a look since this change might have performance impact

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpecUtils.java

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

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

pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/ProjectionBlockValSet.java

...g/apache/pinot/core/query/aggregation/groupby/NoDictionarySingleColumnGroupKeyGenerator.java

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

pinot-core/src/main/java/org/apache/pinot/core/util/QueryOptionsUtils.java

…che#8927)

Jackie-Jiang · 2022-08-10T18:40:58Z

Related to #8697

walterddr · 2022-08-29T14:45:29Z

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

+      for (Object[] row : rows) {
+        for (int i = 0; i < numColumns; i++) {
+          Object columnValue = row[i];
+          if (columnValue == null) {
+            row[i] = colDefaultNullValues[i];
+            nullBitmaps[i].add(rowId);
+          }
+        }
+        rowId++;


can we merge this part with the main loop below? any specific reason we had to loop through the rows one more time?

nizarhejazi added 4 commits June 19, 2022 16:45

Proper null handling in SELECT, ORDER BY, DISTINCT, and GROUP BY

2c15611

merge master

f61f135

minor cleanup

d2326ef

another minor cleanup

574f6f0

nizarhejazi added 2 commits June 20, 2022 15:00

fix tests

bbb0724

update projection block val set

4c29571

Jackie-Jiang reviewed Jun 22, 2022

View reviewed changes

nizarhejazi added 5 commits June 26, 2022 23:17

1) perf optimization; 2) address comments

bf14723

Merge branch 'master' into null_select

93fa0f6

minor cleanup

16170ba

minor cleanup

f0d863c

address lint issue

5e86239

Jackie-Jiang reviewed Jun 30, 2022

View reviewed changes

nizarhejazi added 6 commits July 6, 2022 17:51

merge w/ master

bf31e8d

address comments

ff8cea6

address checkstyle warning

8574a83

refactor; cleanup

0b4f429

Merge branch 'master' into null_select

dbe6d01

address checkstyle warning

39d9a21

Jackie-Jiang reviewed Jul 15, 2022

View reviewed changes

nizarhejazi added 6 commits July 14, 2022 19:05

Merge branch 'master' into null_select

86a3eac

address comments

80f2f44

address one last comment

6882e82

fix linter issue

a64c65a

address comments

73f12d7

address stylecheck warning

c246ec3

Jackie-Jiang reviewed Jul 15, 2022

View reviewed changes

nizarhejazi added 2 commits July 15, 2022 17:20

address comments

e55b5a2

address spotless warning

85e806a

Jackie-Jiang approved these changes Jul 19, 2022

View reviewed changes

Jackie-Jiang merged commit 8acb17f into apache:master Jul 19, 2022

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

yuanbenson pushed a commit to yuanbenson/pinot that referenced this pull request Jul 20, 2022

Proper null handling in SELECT, ORDER BY, DISTINCT, and GROUP BY (apa…

6771c2a

…che#8927)

walterddr reviewed Aug 29, 2022

View reviewed changes

yashmayya added the null support label Sep 6, 2024

Conversation

nizarhejazi commented Jun 19, 2022

Uh oh!

codecov-commenter commented Jun 20, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

nizarhejazi Jul 10, 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!

nizarhejazi Jul 11, 2022

Choose a reason for hiding this comment

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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang commented Aug 10, 2022

Uh oh!

walterddr Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 20, 2022 •

edited

Loading

nizarhejazi Jul 10, 2022 •

edited

Loading