You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "andiem-bq (via GitHub)" <gi...@apache.org> on 2023/02/10 21:55:32 UTC
[GitHub] [pinot] andiem-bq opened a new pull request, #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
andiem-bq opened a new pull request, #10268:
URL: https://github.com/apache/pinot/pull/10268
- In CountAggregateFunction, NPE was thrown when COUNT() is passed a literal and nullHandling is enabled. expression.getIdentifier() returns null when expression is not an identifier.
- Sample failing query:
```
SELECT COUNT("int0") AS "value"
FROM "test"
HAVING (COUNT(1) > 0) option(enableNullHandling=true)
```
Same query can execute when option is not passed.
- Allow nullHandling when COUNT is passed an identifier or a function. Disable when literal or * as these give same result of returning # of rows.
--
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] walterddr commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1105260268
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
shouldn't we also allow literal type. it looks like the test is done on COUNT(1) right?
--
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] andiem-bq commented on pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "andiem-bq (via GitHub)" <gi...@apache.org>.
andiem-bq commented on PR #10268:
URL: https://github.com/apache/pinot/pull/10268#issuecomment-1430290459
Exception thrown when enabling null handling on small data table. Will try to get this test running locally and try to resolve the issue.
```
[{"errorCode":200,"message":"QueryExecutionError:\njava.lang.IllegalStateException: Null handling cannot be enabled for data table version smaller than 4\n\tat com.google.common.base.Preconditions.checkState(Preconditions.java:444)\n\tat
```
--
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] walterddr commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1106184694
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
technically speaking in standardized SQL count(col) returns a number of non-null value count for that column in a table.
- `count(*)` and `count(1)` (or count any non-null literal) are equivalent
- `count(col)` is technically not equivalent to `count(*)`
- when nullHandlingEnabled = false. it is equivalent to `count(*)`
- when nullHandlingEnabled = true. i am not sure if the current result is correct (but this is out of scope of this PR)
so my suggestion is
```suggestion
|| (expression.getType() == ExpressionContext.Type.FUNCTION)
|| (expression.getType() == ExpressionContext.Type.LITERAL && expression.getLiteral().getValue() != null));
```
--
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] walterddr commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1106548320
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
not necssarily b/c we have compile time function invoker that can produce a NULL. but agreed we can address separately.
--
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] 61yao commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1106579512
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
What's the motivation of adding function to enable null count?
The behavior should be :
When nullHandling is enabled
count() should ignore all nulls exception it is count(*)
--
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] andiem-bq commented on pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "andiem-bq (via GitHub)" <gi...@apache.org>.
andiem-bq commented on PR #10268:
URL: https://github.com/apache/pinot/pull/10268#issuecomment-1428528718
Added test under NullHandlingIntegrationTest
--
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] andiem-bq commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "andiem-bq (via GitHub)" <gi...@apache.org>.
andiem-bq commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1106108192
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
My thought was to give COUNT(literal) same treatment as COUNT(*) as these give same result of returning total # of rows in a collection regardless of nulls.
But main goal of fix was to avoid the getIdentifier.equals() NPE. Should I revise the statement to allow null handling for identifiers that are not * and all non-identifiers?
--
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 #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1106208200
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
With the change the only scenario not handled is `count(null)`, which IMO is a useless operation because it always return 0.
The fix is basically just preventing the NPE from the '*' check
--
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] andiem-bq commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "andiem-bq (via GitHub)" <gi...@apache.org>.
andiem-bq commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1107414022
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
Previous behaviour was count ignore all nulls for all expressions that are identifiers that are not * . Expressions that are not identifiers (literals or functions) result in exception because of the old check.
New behaviour is ignore all nulls for all expressions that are not count (*) or count(any literal). count(*) and count(literal) both return rows. It misses count(null) case tho.
--
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 #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10268:
URL: https://github.com/apache/pinot/pull/10268#issuecomment-1428850189
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10268](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d214748) into [master](https://codecov.io/gh/apache/pinot/commit/dfc84a529b5b5b3fbfd3a9b8884249478f5d54e0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dfc84a5) will **decrease** coverage by `56.68%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10268 +/- ##
=============================================
- Coverage 70.40% 13.73% -56.68%
+ Complexity 5109 182 -4927
=============================================
Files 2017 1962 -55
Lines 109293 106867 -2426
Branches 16615 16326 -289
=============================================
- Hits 76952 14677 -62275
- Misses 26943 91035 +64092
+ Partials 5398 1155 -4243
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.73% <0.00%> (+0.02%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...aggregation/function/CountAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9Db3VudEFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-78.75%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/common/CustomObject.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vQ3VzdG9tT2JqZWN0LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1593 more](https://codecov.io/gh/apache/pinot/pull/10268?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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=The+Apache+Software+Foundation)
--
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] walterddr commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1106186151
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
not sure if this is necessary now since we don't have null literal support properly. CC @61yao please share your 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 merged pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10268:
URL: https://github.com/apache/pinot/pull/10268
--
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 #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1105095502
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -186,4 +186,11 @@ public void testCaseWithIsNotDistinctFrom()
query = "SELECT description FROM " + getTableName() + " where description IS NOT DISTINCT FROM description";
testQuery(query);
}
+
+ @Test
+ public void testTotalCountUsingCountLiteral()
+ throws Exception {
+ String query = "SELECT COUNT(1) FROM " + getTableName();
Review Comment:
I think you need to put `SET enableNullHandling = true;` to verify the desired behavior.
--
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] andiem-bq commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "andiem-bq (via GitHub)" <gi...@apache.org>.
andiem-bq commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1107414022
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
Previous behaviour was count ignore all nulls for all expressions that are identifiers that are not * . Expressions that are not identifiers (literals or functions) result in exception because of the old check.
New behaviour is ignore all nulls for all expressions that are not count ( * ) or count(any literal). count( * ) and count(literal) both return rows. It misses count(null) case tho.
--
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] andiem-bq commented on a diff in pull request #10268: Resolve NullPointerException when COUNT() is passed literal and nullHandling is enabled
Posted by "andiem-bq (via GitHub)" <gi...@apache.org>.
andiem-bq commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1107414022
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext expression) {
public CountAggregationFunction(ExpressionContext expression, boolean nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled && !expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER && !expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
Previous behaviour was count ignore all nulls for all expressions that are identifiers that are not *. Expressions that are not identifiers (literals or functions) result in exception because of the old check.
New behaviour is ignore all nulls for all expressions that are not count (*) or count(any literal). count(*) and count(literal) both return rows. It misses count(null) case tho.
--
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