You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2023/08/20 11:45:35 UTC
[GitHub] [pinot] abhioncbr opened a new pull request, #11395: [multistage]: CASE/WHEN query with NULL support
abhioncbr opened a new pull request, #11395:
URL: https://github.com/apache/pinot/pull/11395
This PR is for the [issue](https://github.com/apache/pinot/issues/10513)
- enables `ENABLE_NULL_HANDLING` for multistage query testing.
- Added util function to skip the evaluation of the `valueBlock` in case of null Literal transform function.
- Using the above util function in case transformation function.
- Added multiple case/when queries with null literal function case.
cc: @shenyu0127, @Jackie-Jiang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr closed pull request #11395: [multistage]: CASE/WHEN query with NULL support
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr closed pull request #11395: [multistage]: CASE/WHEN query with NULL support
URL: https://github.com/apache/pinot/pull/11395
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #11395: CASE/WHEN query with NULL support
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11395:
URL: https://github.com/apache/pinot/pull/11395#discussion_r1299490599
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:
##########
@@ -331,7 +331,7 @@ protected int[] transformToIntValuesSVUsingValue(ValueBlock valueBlock) {
}
}
if (!unselectedDocs.isEmpty()) {
- if (_elseStatement == null) {
+ if (_elseStatement == null || LiteralTransformFunction.isNullLiteralTransform(_elseStatement)) {
Review Comment:
To simplify this, we can change `constructStatementListCalcite()` (also change `constructStatementListLegacy()` for backward compatible) to set `_elseStatement` to `null` when it is null literal
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java:
##########
@@ -269,4 +277,15 @@ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
bitmap.add(0L, length);
return bitmap;
}
+
+ /*
+ * Util function to check whether the Literal transform is null or not.
+ */
+ public static boolean isNullLiteralTransform(TransformFunction function) {
Review Comment:
Suggest adding `boolean isNull()` instead. Checking `_dataType` itself should be enough (consistent with `getNullBitmap()`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #11395: CASE/WHEN query with NULL support
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #11395:
URL: https://github.com/apache/pinot/pull/11395#issuecomment-1686721338
> > > 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #11395: Tests for CASE/WHEN query with NULL support
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11395:
URL: https://github.com/apache/pinot/pull/11395
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] shenyu0127 commented on pull request #11395: CASE/WHEN query with NULL support
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on PR #11395:
URL: https://github.com/apache/pinot/pull/11395#issuecomment-1686643981
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #11395: [multistage]: CASE/WHEN query with NULL support
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11395:
URL: https://github.com/apache/pinot/pull/11395#issuecomment-1685271390
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11395?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11395](https://app.codecov.io/gh/apache/pinot/pull/11395?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (bb62ba7) into [master](https://app.codecov.io/gh/apache/pinot/commit/575398dae0841ba152c31306cb460cfdc776b04b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (575398d) will **decrease** coverage by `46.91%`.
> Report is 11 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11395 +/- ##
=============================================
- Coverage 61.46% 14.56% -46.91%
+ Complexity 6514 495 -6019
=============================================
Files 2233 2159 -74
Lines 120144 116981 -3163
Branches 18234 17788 -446
=============================================
- Hits 73848 17036 -56812
- Misses 40882 98532 +57650
+ Partials 5414 1413 -4001
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.56% <0.00%> (-46.77%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.56% <0.00%> (-46.91%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.56% <0.00%> (-0.04%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ator/transform/function/CaseTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQ2FzZVRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-38.40%)` | :arrow_down: |
| [...r/transform/function/LiteralTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTGl0ZXJhbFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-73.92%)` | :arrow_down: |
... and [1470 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11395/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] shenyu0127 commented on pull request #11395: CASE/WHEN query with NULL support
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on PR #11395:
URL: https://github.com/apache/pinot/pull/11395#issuecomment-1686753556
> > > > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #11395: CASE/WHEN query with NULL support
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #11395:
URL: https://github.com/apache/pinot/pull/11395#issuecomment-1687224512
As suggested, diving this PR into two
- for enabling the ignored query
- for performance changes.
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] shenyu0127 commented on pull request #11395: CASE/WHEN query with NULL support
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on PR #11395:
URL: https://github.com/apache/pinot/pull/11395#issuecomment-1685709261
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #11395: CASE/WHEN query with NULL support
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #11395:
URL: https://github.com/apache/pinot/pull/11395#issuecomment-1686618006
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org