Skip to content

Add support for IS NULL and NOT IS NULL in transform functions #8264

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.

Sign up for GitHub

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

Jump to bottom
Merged
Jackie-Jiang merged 14 commits into from
Mar 22, 2022
Merged

Add support for IS NULL and NOT IS NULL in transform functions #8264

Jackie-Jiang merged 14 commits into from
Mar 22, 2022

Conversation

@KKcorps
Copy link
Contributor

@KKcorps KKcorps commented Feb 28, 2022 •
edited
Loading

This PR addresses the issue #8251 . Currently, Pinot supports IS NULL syntax in filter expressions such as WHERE clause. But it doesn't support as part of SELECT expressions such as CASE WHEN col IS NULL THEN a ELSE b

The PR adds support for such expressions by utilising the NullValueVectorReader to get the docIds which are null.

The implementation has two requirements/limitations -

  • nullHandlingEnabled should be set to true since native NULL values are not supported in Pinot and requires a null value bitmap index.

  • Only column names can be provided as the first argument for expressions. The support for functions is still not there.
    e.g. colA IS NULL is supported but LOWER(colA) IS NULL is not supported.

Currently Pending -

  • Documentation

@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2022 •
edited
Loading

Codecov Report

Merging #8264 (f0e7618) into master (25cd6e3) will decrease coverage by 5.72%.
The diff coverage is 64.70%.

@@             Coverage Diff              @@
##             master    #8264      +/-   ##
============================================
- Coverage     69.72%   64.00%   -5.73%     
- Complexity     4264     4272       +8     
============================================
  Files          1639     1600      -39     
  Lines         85920    84243    -1677     
  Branches      12922    12737     -185     
============================================
- Hits          59906    53918    -5988     
- Misses        21842    26431    +4589     
+ Partials       4172     3894     -278
Flag Coverage Δ
integration1 ?
unittests1 66.92% <64.70%> (-0.03%) ⬇️
unittests2 14.18% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...r/transform/function/TransformFunctionFactory.java 80.90% <60.00%> (-2.73%) ⬇️
...transform/function/IsNotNullTransformFunction.java 63.33% <63.33%> (ø)
...or/transform/function/IsNullTransformFunction.java 63.33% <63.33%> (ø)
...e/pinot/common/function/TransformFunctionType.java 100.00% <100.00%> (ø)
...ransform/function/IdentifierTransformFunction.java 72.72% <100.00%> (+0.63%) ⬆️
...va/org/apache/pinot/common/config/NettyConfig.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java 0.00% <0.00%> (-100.00%) ⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/BrokerQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/MinionQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
... and 353 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 25cd6e3...f0e7618. Read the comment docs.

@KKcorps KKcorps marked this pull request as ready for review March 18, 2022 12:57
@KKcorps KKcorps requested a review from richardstartin March 18, 2022 13:05
@KKcorps KKcorps force-pushed the case_is_null_patch branch from 983a336 to 44744cf Compare March 18, 2022 13:10
richardstartin
richardstartin approved these changes Mar 18, 2022
View reviewed changes
Copy link
Member

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

👍🏻

Jackie-Jiang
Jackie-Jiang reviewed Mar 18, 2022
View reviewed changes
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.

Suggest also add some tests in NullHandlingIntegrationTest

@KKcorps
Copy link
Contributor Author

KKcorps commented Mar 19, 2022

One more side note - If these functions are used with a table where Null handling is disabled, they assume everything is non-null. So IS NULL will always return false and IS NOT NULL will always return true.

Instead, If you want it to throw exception in this case, I can do that as well.

@Jackie-Jiang
Copy link
Contributor

Jackie-Jiang commented Mar 21, 2022

One more side note - If these functions are used with a table where Null handling is disabled, they assume everything is non-null. So IS NULL will always return false and IS NOT NULL will always return true.

This is the correct behavior which aligns with the filter semantic

@Jackie-Jiang Jackie-Jiang merged commit b345357 into apache:master Mar 22, 2022
@walterddr walterddr mentioned this pull request Sep 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardstartin richardstartin richardstartin approved these changes

@Jackie-Jiang Jackie-Jiang Jackie-Jiang left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KKcorps @codecov-commenter @Jackie-Jiang @richardstartin