Tests for CASE/WHEN query with NULL support by abhioncbr · Pull Request #11395 · apache/pinot

abhioncbr · 2023-08-20T11:45:34Z

This PR is for the issue

enables ENABLE_NULL_HANDLING for multistage query testing.
Added multiple case/when queries with null literal function case.

codecov-commenter · 2023-08-20T12:23:51Z

Codecov Report

Merging #11395 (43c6c2c) into master (575398d) will increase coverage by 1.41%.
Report is 17 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #11395      +/-   ##
============================================
+ Coverage     61.46%   62.88%   +1.41%     
+ Complexity     6514      207    -6307     
============================================
  Files          2233     2300      +67     
  Lines        120144   123622    +3478     
  Branches      18234    18796     +562     
============================================
+ Hits          73848    77740    +3892     
+ Misses        40882    40356     -526     
- Partials       5414     5526     +112

Flag	Coverage Δ
integration	`<0.01% <ø> (?)`
integration1	`<0.01% <ø> (+<0.01%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`62.86% <ø> (+1.41%)`	⬆️
java-17	`49.73% <ø> (-11.57%)`	⬇️
java-20	`62.74% <ø> (+1.41%)`	⬆️
temurin	`62.88% <ø> (+1.41%)`	⬆️
unittests	`62.88% <ø> (?)`
unittests1	`67.40% <ø> (+0.40%)`	⬆️
unittests2	`14.51% <ø> (-0.08%)`	⬇️

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

see 347 files with indirect coverage changes

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

Jackie-Jiang

@shenyu0127 Please also take a look

Jackie-Jiang · 2023-08-21T01:21:47Z

...e/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java

    }
    if (!unselectedDocs.isEmpty()) {
-      if (_elseStatement == null) {
+      if (_elseStatement == null || LiteralTransformFunction.isNullLiteralTransform(_elseStatement)) {


To simplify this, we can change constructStatementListCalcite() (also change constructStatementListLegacy() for backward compatible) to set _elseStatement to null when it is null literal

Jackie-Jiang · 2023-08-21T01:26:48Z

...rc/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java

+  /*
+   * Util function to check whether the Literal transform is null or not.
+   */
+  public static boolean isNullLiteralTransform(TransformFunction function) {


Suggest adding boolean isNull() instead. Checking _dataType itself should be enough (consistent with getNullBitmap())

shenyu0127 · 2023-08-21T06:10:10Z

Without this PR, the query SELECT CASE WHEN boolCol = true THEN 'Yes' also returns NULL if boolCol is NULL or false. Is the purpose of this PR only to improve performance? If that's the case, we can simply re-enable the ignored query.

abhioncbr · 2023-08-21T16:10:17Z

Without this PR, the query SELECT CASE WHEN boolCol = true THEN 'Yes' also returns NULL if boolCol is NULL or false. Is the purpose of this PR only to improve performance? If that's the case, we can simply re-enable the ignored query.

Initially, when I started looking a couple of months back, it was an issue, but as of now, it is more or less a performance improvement and enabling the ignored queries. Please let me know if we can drop the performance improvement changes. Thanks

shenyu0127 · 2023-08-21T16:25:33Z

Without this PR, the query SELECT CASE WHEN boolCol = true THEN 'Yes' also returns NULL if boolCol is NULL or false. Is the purpose of this PR only to improve performance? If that's the case, we can simply re-enable the ignored query.

Initially, when I started looking a couple of months back, it was an issue, but as of now, it is more or less a performance improvement and enabling the ignored queries. Please let me know if we can drop the performance improvement changes. Thanks

I see. Please drop the performance improvement changes.

abhioncbr · 2023-08-21T17:12:00Z

Without this PR, the query SELECT CASE WHEN boolCol = true THEN 'Yes' also returns NULL if boolCol is NULL or false. Is the purpose of this PR only to improve performance? If that's the case, we can simply re-enable the ignored query.

Initially, when I started looking a couple of months back, it was an issue, but as of now, it is more or less a performance improvement and enabling the ignored queries. Please let me know if we can drop the performance improvement changes. Thanks

I see. Please drop the performance improvement changes.

Just out of curiosity, any particular reason for not having the performance logic?

shenyu0127 · 2023-08-21T17:36:26Z

Without this PR, the query SELECT CASE WHEN boolCol = true THEN 'Yes' also returns NULL if boolCol is NULL or false. Is the purpose of this PR only to improve performance? If that's the case, we can simply re-enable the ignored query.

Initially, when I started looking a couple of months back, it was an issue, but as of now, it is more or less a performance improvement and enabling the ignored queries. Please let me know if we can drop the performance improvement changes. Thanks

I see. Please drop the performance improvement changes.

Just out of curiosity, any particular reason for not having the performance logic?

We prefer one PR to do one simple thing.

If it's to fix the bug and re-enable the ignored query, the two changes are closely related and can be put one PR.
If it's to improve performance and re-enable the ignored query, the two changes are not related and can be put in two PRs separately. Putting these two things in one PR causes confusion to the reviewer.

abhioncbr · 2023-08-22T00:09:18Z

As suggested, diving this PR into two

for enabling the ignored query
for performance changes.
Thanks

Jackie-Jiang

LGTM

Could you revise the title and description for the PR accordingly?

shenyu0127

LGTM

abhioncbr added 5 commits August 16, 2023 20:46

initial commit

002720c

Merge branch 'master' into literal-null-support

c95f716

Skipping null lieral trasnformation in case queries.

2c15cc2

reverting unwanted changes for local testing.

a97a3f7

Added unit test case for null LiteralTransform util function.

bb62ba7

abhioncbr closed this Aug 20, 2023

abhioncbr reopened this Aug 20, 2023

Jackie-Jiang added bugfix multi-stage Related to the multi-stage query engine and removed multi-stage Related to the multi-stage query engine labels Aug 21, 2023

Jackie-Jiang changed the title ~~[multistage]: CASE/WHEN query with NULL support~~ CASE/WHEN query with NULL support Aug 21, 2023

Jackie-Jiang reviewed Aug 21, 2023

View reviewed changes

Changes as per the PR comments.

43c6c2c

Jackie-Jiang added the testing label Aug 22, 2023

Jackie-Jiang approved these changes Aug 22, 2023

View reviewed changes

abhioncbr changed the title ~~CASE/WHEN query with NULL support~~ Tests for CASE/WHEN query with NULL support Aug 22, 2023

shenyu0127 approved these changes Aug 22, 2023

View reviewed changes

Jackie-Jiang merged commit adbfd23 into apache:master Aug 22, 2023

abhioncbr mentioned this pull request Aug 25, 2023

Skip computation incase of Null Literal for case/when transformation #11444

Merged

abhioncbr deleted the literal-null-support branch April 25, 2024 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Tests for CASE/WHEN query with NULL support#11395

Tests for CASE/WHEN query with NULL support#11395
Jackie-Jiang merged 6 commits intoapache:masterfrom
abhioncbr:literal-null-support

abhioncbr commented Aug 20, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Aug 20, 2023 •

edited

Loading

Uh oh!

Jackie-Jiang left a comment

Uh oh!

Jackie-Jiang Aug 21, 2023

Uh oh!

Jackie-Jiang Aug 21, 2023

Uh oh!

shenyu0127 commented Aug 21, 2023

Uh oh!

abhioncbr commented Aug 21, 2023

Uh oh!

shenyu0127 commented Aug 21, 2023

Uh oh!

abhioncbr commented Aug 21, 2023

Uh oh!

shenyu0127 commented Aug 21, 2023

Uh oh!

abhioncbr commented Aug 22, 2023

Uh oh!

Jackie-Jiang left a comment

Uh oh!

shenyu0127 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

abhioncbr commented Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 20, 2023 • 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!

Jackie-Jiang Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

shenyu0127 commented Aug 21, 2023

Uh oh!

abhioncbr commented Aug 21, 2023

Uh oh!

shenyu0127 commented Aug 21, 2023

Uh oh!

abhioncbr commented Aug 21, 2023

Uh oh!

shenyu0127 commented Aug 21, 2023

Uh oh!

abhioncbr commented Aug 22, 2023

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

shenyu0127 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abhioncbr commented Aug 20, 2023 •

edited

Loading

codecov-commenter commented Aug 20, 2023 •

edited

Loading