Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a null related error in queries like: #11829

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

gortiz
Copy link
Contributor

@gortiz gortiz commented Oct 18, 2023

We have detected that queries like

SELECT col1, col2
FROM testTable 
ORDER BY col1 
option(enableNullHandling=true)

were incorrectly executed. Specifically, the problem was that null bitmap was not applied on projected columns that were not in the order by expression in SelectionOrderByOperator.computePartiallyOrdered.

In the previous example, for each null value in col2, Pinot returns the default null value for that column instead of null. This patch fixes that and adds some regression tests.

@gortiz gortiz force-pushed the fix-order-not-sorted-null-values branch 2 times, most recently from 6c6baf5 to 9374b7d Compare October 18, 2023 10:30
queryContext.setNullHandlingEnabled(true);
List<Object[]> rows = executeQuery(queryContext);
assertNull(rows.get(0)[0], "Column 'col1' value should be 'null' when null handling is enabled");
assertNull(rows.get(0)[1], "Column 'col2' value should be 'null' when null handling is enabled");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this path, this line failed with:

java.lang.AssertionError: Column 'col2' value should be 'null' when null handling is enabled
Expected :null
Actual   :-2147483648

```
select salary
from mytable
where salary is null
order by description
option(enableNullHandling=true)
```

Which didn't return null values for not ordered columns
@gortiz gortiz force-pushed the fix-order-not-sorted-null-values branch from 9374b7d to c1cda08 Compare October 18, 2023 10:37
@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2023

Codecov Report

Merging #11829 (c1cda08) into master (9378b06) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master   #11829   +/-   ##
=========================================
  Coverage     66.48%   66.49%           
  Complexity      207      207           
=========================================
  Files          2343     2343           
  Lines        126418   126428   +10     
  Branches      19440    19445    +5     
=========================================
+ Hits          84054    84064   +10     
  Misses        36499    36499           
  Partials       5865     5865           
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 66.44% <100.00%> (+0.01%) ⬆️
java-17 <0.01% <0.00%> (-66.35%) ⬇️
java-20 66.35% <100.00%> (+0.03%) ⬆️
temurin 66.49% <100.00%> (+<0.01%) ⬆️
unittests 66.49% <100.00%> (+<0.01%) ⬆️
unittests1 67.35% <100.00%> (+<0.01%) ⬆️
unittests2 17.71% <0.00%> (-0.01%) ⬇️

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

Files Coverage Δ
.../core/operator/query/SelectionOrderByOperator.java 97.57% <100.00%> (+0.15%) ⬆️

... and 15 files with indirect coverage changes

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

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@Jackie-Jiang Jackie-Jiang merged commit 86d3c44 into apache:master Oct 19, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants