You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shenyu0127 (via GitHub)" <gi...@apache.org> on 2023/07/26 22:10:05 UTC
[GitHub] [pinot] shenyu0127 opened a new pull request, #11188: Fail the query when IN or NOT IN filter contains NULL.
shenyu0127 opened a new pull request, #11188:
URL: https://github.com/apache/pinot/pull/11188
(no comment)
--
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 #11188: Fail the query when IN or NOT IN filter contains NULL.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11188:
URL: https://github.com/apache/pinot/pull/11188#issuecomment-1652661966
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11188?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11188](https://app.codecov.io/gh/apache/pinot/pull/11188?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0167f4c) into [master](https://app.codecov.io/gh/apache/pinot/commit/e87a05045d3aef04d2978868a0b6d79f38add51c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e87a050) will **increase** coverage by `0.00%`.
> Report is 4 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11188 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2214 2163 -51
Lines 118822 116610 -2212
Branches 17978 17718 -260
=========================================
Hits 137 137
+ Misses 118665 116453 -2212
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration2temurin11 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin17 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin20 | `?` | |
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/11188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://app.codecov.io/gh/apache/pinot/pull/11188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `0.00% <0.00%> (ø)` | |
... and [80 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11188/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 #11188: Fail the query when IN or NOT IN filter contains NULL.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on PR #11188:
URL: https://github.com/apache/pinot/pull/11188#issuecomment-1654503145
> @shenyu0127 : can you share what was the previous behavior was for these queries?
The previous behavior was to throw a different error from segment pruning:
Currently the query `SELECT * FROM airlineStats WHERE AirTime IN (1, 2, null)` gets the following error
```
[
{
"errorCode": 200,
"message": "QueryExecutionError:\norg.apache.pinot.spi.exception.BadQueryRequestException: java.lang.IllegalArgumentException: Cannot convert value: 'null' to type: INT\n\tat org.apache.pinot.core.query.pruner.ValueBasedSegmentPruner.convertValue(ValueBasedSegmentPruner.java:157)\n\tat org.apache.pinot.core.query.pruner.ValueBasedSegmentPruner$ValueCache$CachedValue.ensureDataType(ValueBasedSegmentPruner.java:223)\n\tat org.apache.pinot.core.query.pruner.ValueBasedSegmentPruner$ValueCache.get(ValueBasedSegmentPruner.java:197)\n\tat org.apache.pinot.core.query.pruner.ColumnValueSegmentPruner.pruneInPredicate(ColumnValueSegmentPruner.java:147)\n...\nCaused by: java.lang.IllegalArgumentException: Cannot convert value: 'null' to type: INT\n\tat org.apache.pinot.spi.data.FieldSpec$DataType.convertInternal(FieldSpec.java:565)\n\tat org.apache.pinot.core.query.pruner.ValueBasedSegmentPruner.convertValue(ValueBasedSegmentPruner.java:155)\n\t... 22 more"
}
]
```
--
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 a diff in pull request #11188: Fail the query if a filter's rhs contains NULL.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #11188:
URL: https://github.com/apache/pinot/pull/11188#discussion_r1278232062
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -267,6 +270,35 @@ private static void validateDistinctQuery(PinotQuery pinotQuery)
}
}
+ /*
+ * Throws an exception if the filter's rhs has NULL because:
+ * - Predicate evaluator and pruning do not have NULL support.
+ * - It is not meaningful to have NULL in the filter's rhs.
+ * - For most of the filters (e.g. GREATER_THAN, LIKE), the rhs being NULL leads to no record matched.
+ * - For IN, adding NULL to the rhs list does not change the matched records.
+ * - For NOT IN, adding NULL to the rhs list leads to no record matched.
+ * - For "= NULL" and "!= NULL", we have the IS_NULL and IS_NOT_NULL as workarounds.
Review Comment:
You are right. Corrected the function comment.
--
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 #11188: Fail the query if a filter's rhs contains NULL..
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on PR #11188:
URL: https://github.com/apache/pinot/pull/11188#issuecomment-1656532921
> Can you also check if we have proper behavior on `= NULL`, `!= NULL`, `> NULL` etc? Probably we should avoid explicit `NULL` in all the predicate?
Good point. Changed the PR to avoid explicit `NULL` in all the predicates.
> Please also add some description to the PR and some comment in the code explaining why we don't allow NULL in IN/NOT IN
Added a function comment.
--
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 #11188: Fail the query if a filter's rhs contains NULL..
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11188:
URL: https://github.com/apache/pinot/pull/11188#discussion_r1278228727
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -267,6 +270,35 @@ private static void validateDistinctQuery(PinotQuery pinotQuery)
}
}
+ /*
+ * Throws an exception if the filter's rhs has NULL because:
+ * - Predicate evaluator and pruning do not have NULL support.
+ * - It is not meaningful to have NULL in the filter's rhs.
+ * - For most of the filters (e.g. GREATER_THAN, LIKE), the rhs being NULL leads to no record matched.
+ * - For IN, adding NULL to the rhs list does not change the matched records.
+ * - For NOT IN, adding NULL to the rhs list leads to no record matched.
+ * - For "= NULL" and "!= NULL", we have the IS_NULL and IS_NOT_NULL as workarounds.
Review Comment:
Semantically `= NULL` and `!= NULL` is not the same as `IS_NULL` and `IS_NOT_NULL`. They will always return `null` which is not really useful
--
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 #11188: Fail the query if a filter's rhs contains NULL.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11188:
URL: https://github.com/apache/pinot/pull/11188
--
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 pull request #11188: Fail the query when IN or NOT IN filter contains NULL.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11188:
URL: https://github.com/apache/pinot/pull/11188#issuecomment-1656157072
Please also add some description to the PR and some comment in the code explaining why we don't allow NULL in IN/NOT IN
--
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] ankitsultana commented on pull request #11188: Fail the query when IN or NOT IN filter contains NULL.
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on PR #11188:
URL: https://github.com/apache/pinot/pull/11188#issuecomment-1654247315
@shenyu0127 : can you share what was the previous behavior was for these queries?
--
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