You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2024/10/31 09:56:52 UTC
[PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
gortiz opened a new pull request, #14342:
URL: https://github.com/apache/pinot/pull/14342
This PR modifies AggregationPlanNode to improve performance when null handling is enabled for the query but the involved columns do not contain nulls for this specific segment.
For example, a query like:
```sql
set enableNullHandling=true;
set explainAskingServers=true;
explain plan for
select count(deviceOS) from userAttributes limit 10
```
Where `deviceOS` is nullable returned:
```
LogicalSort(fetch=[10])
PinotLogicalSortExchange(distribution=[hash], collation=[[]], isSortOnSender=[false], isSortOnReceiver=[false])
PinotLogicalAggregate(group=[{}], agg#0=[COUNT($0)])
PinotLogicalExchange(distribution=[hash])
LeafStageCombineOperator(table=[userAttributes])
StreamingInstanceResponse
CombineAggregate
Aggregate(aggregations=[[count(deviceOS)]]) <-- see this
Project(columns=[[deviceOS]])
DocIdSet(maxDocs=[40000])
FilterMatchEntireSegment(numDocs=[10000])
```
While with this PR it returns:
```
LogicalSort(fetch=[10])
PinotLogicalSortExchange(distribution=[hash], collation=[[]], isSortOnSender=[false], isSortOnReceiver=[false])
PinotLogicalAggregate(group=[{}], agg#0=[COUNT($0)])
PinotLogicalExchange(distribution=[hash])
LeafStageCombineOperator(table=[userAttributes])
StreamingInstanceResponse
CombineAggregate
FastFilteredCount <-- see this
FilterMatchEntireSegment(numDocs=[10000])
```
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "bziobrowski (via GitHub)" <gi...@apache.org>.
bziobrowski commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1824432074
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,38 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ List<?> inputExpressions = aggregationFunction.getInputExpressions();
+ if (inputExpressions.isEmpty()) {
+ continue;
+ }
Review Comment:
What if aggregation function has more than one input expression ?
btw, it'd be good to add some test(s).
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "bziobrowski (via GitHub)" <gi...@apache.org>.
bziobrowski commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1824432074
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,38 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ List<?> inputExpressions = aggregationFunction.getInputExpressions();
+ if (inputExpressions.isEmpty()) {
+ continue;
+ }
Review Comment:
What if aggregation function has more than one input expression ?
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1824309012
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,34 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ ExpressionContext argument = (ExpressionContext) aggregationFunction.getInputExpressions().get(0);
+ switch (argument.getType()) {
+ case IDENTIFIER:
+ DataSource dataSource = _indexSegment.getDataSource(argument.getIdentifier());
+ NullValueVectorReader nullValueVector = dataSource.getNullValueVector();
+ if (nullValueVector != null && !nullValueVector.getNullBitmap().isEmpty()) {
+ return true;
+ }
+ break;
+ case LITERAL:
+ return false;
Review Comment:
> The non-scan based aggregation operator is only chosen if the aggregation function input operand is an identifier and for fast filtered count, it shouldn't matter because COUNT(literal) is treated the same as COUNT(*)
But that is the fast literal and non scan limitation and responsibility. This functions returns whether there are nulls or not. If in the future we change the requirements say non scan aggregations, we shouldn't need to modify this code.
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #14342:
URL: https://github.com/apache/pinot/pull/14342#issuecomment-2449462886
cc @bziobrowski
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1827480623
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,38 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ List<?> inputExpressions = aggregationFunction.getInputExpressions();
+ if (inputExpressions.isEmpty()) {
+ continue;
+ }
Review Comment:
Oh! But that was on a different line :laughing:.
Formally, you are right. But then `isFitForNonScanBasedPlan` is doing the same thing, so it has no side effect. Anyway, in order to decouple both methods, it would be better to check all inputs. I'm changing that.
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #14342:
URL: https://github.com/apache/pinot/pull/14342
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1827448259
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,38 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ List<?> inputExpressions = aggregationFunction.getInputExpressions();
+ if (inputExpressions.isEmpty()) {
+ continue;
+ }
Review Comment:
> What if aggregation function has more than one input expression ?
I don't get it. Then all of them need to fulfill the conditions imposed here. This method returns true if and only if we are sure all inputs for all aggregation functions are not nullable. We cannot be sure in the case of functions and an aggregation function without inputs trivially fulfill this requirement.
> btw, it'd be good to add some test(s).
This is actually being tested: https://app.codecov.io/gh/apache/pinot/pull/14342/blob/pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1824307112
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,34 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ ExpressionContext argument = (ExpressionContext) aggregationFunction.getInputExpressions().get(0);
+ switch (argument.getType()) {
+ case IDENTIFIER:
+ DataSource dataSource = _indexSegment.getDataSource(argument.getIdentifier());
+ NullValueVectorReader nullValueVector = dataSource.getNullValueVector();
+ if (nullValueVector != null && !nullValueVector.getNullBitmap().isEmpty()) {
+ return true;
+ }
+ break;
+ case LITERAL:
+ return false;
Review Comment:
I guess we can transform this into:
```java
return argument.getLiteral().isNull();
```
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1827448259
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,38 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ List<?> inputExpressions = aggregationFunction.getInputExpressions();
+ if (inputExpressions.isEmpty()) {
+ continue;
+ }
Review Comment:
> What if aggregation function has more than one input expression ?
I don't get it. Then all of them need to fulfill the conditions imposed here. This method returns false if and only if we are sure all inputs for all aggregation functions are not nullable. We cannot be sure in the case of functions and an aggregation function without inputs trivially fulfill this requirement.
> btw, it'd be good to add some test(s).
This is actually being tested: https://app.codecov.io/gh/apache/pinot/pull/14342/blob/pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14342:
URL: https://github.com/apache/pinot/pull/14342#issuecomment-2449533676
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14342?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ea03f93`)](https://app.codecov.io/gh/apache/pinot/commit/ea03f932abe971b1999345ce62fcae1852f75d3e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1268 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (ea03f93). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (ea03f93) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|0|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14342 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 3 -2433
Lines 133233 6 -133227
Branches 20636 0 -20636
=============================================
- Hits 82274 0 -82274
+ Misses 44911 6 -44905
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14342/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/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-27.73%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14342/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/14342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14342?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1824229868
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,34 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ ExpressionContext argument = (ExpressionContext) aggregationFunction.getInputExpressions().get(0);
+ switch (argument.getType()) {
+ case IDENTIFIER:
+ DataSource dataSource = _indexSegment.getDataSource(argument.getIdentifier());
+ NullValueVectorReader nullValueVector = dataSource.getNullValueVector();
+ if (nullValueVector != null && !nullValueVector.getNullBitmap().isEmpty()) {
+ return true;
+ }
+ break;
+ case LITERAL:
+ return false;
Review Comment:
This could potentially be the `null` literal value too but looks like this doesn't matter for both the cases? The non-scan based aggregation operator is only chosen if the aggregation function input operand is an identifier and for fast filtered count, it shouldn't matter because `COUNT(literal)` is treated the same as `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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "bziobrowski (via GitHub)" <gi...@apache.org>.
bziobrowski commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1827473958
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,38 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ List<?> inputExpressions = aggregationFunction.getInputExpressions();
+ if (inputExpressions.isEmpty()) {
+ continue;
+ }
Review Comment:
What I meant is - shouldn't the function be checking all input expressions instead of only the first one ?
```java
inputExpressions.get(0);
```
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "bziobrowski (via GitHub)" <gi...@apache.org>.
bziobrowski commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1827473958
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,38 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ List<?> inputExpressions = aggregationFunction.getInputExpressions();
+ if (inputExpressions.isEmpty()) {
+ continue;
+ }
Review Comment:
What I meant is - shouldn't the function be checking all input expressions instead of only the first one ?
```java
inputExpressions.get(0);
```
As for testing - is there a test asserting a plan similar to the one you mention in PR description ?
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1827450200
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,38 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ List<?> inputExpressions = aggregationFunction.getInputExpressions();
+ if (inputExpressions.isEmpty()) {
+ continue;
+ }
Review Comment:
Ups, there is a bug in the case it is a literal
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1824231913
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,34 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ ExpressionContext argument = (ExpressionContext) aggregationFunction.getInputExpressions().get(0);
+ switch (argument.getType()) {
+ case IDENTIFIER:
+ DataSource dataSource = _indexSegment.getDataSource(argument.getIdentifier());
+ NullValueVectorReader nullValueVector = dataSource.getNullValueVector();
+ if (nullValueVector != null && !nullValueVector.getNullBitmap().isEmpty()) {
+ return true;
+ }
+ break;
+ case LITERAL:
+ return false;
Review Comment:
Hm, looks like Postgres and MySQL return `0` for `COUNT(null)` interestingly. We might want to do the same although I doubt it's a valid use case anyone cares about (and is also orthogonal to this PR).
--
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
Re: [PR] Modify AggregationPlanNode to consider not nullable columns that do not contain nulls [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #14342:
URL: https://github.com/apache/pinot/pull/14342#discussion_r1824330008
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -118,6 +120,34 @@ public Operator<AggregationResultsBlock> buildNonFilteredAggOperator() {
return new AggregationOperator(_queryContext, aggregationInfo, numTotalDocs);
}
+ /**
+ * Returns {@code true} if any of the aggregation functions have null values, {@code false} otherwise.
+ *
+ * The current implementation is pessimistic and returns {@code true} if any of the arguments to the aggregation
+ * functions is of function type. This is because we do not have a way to determine if the function will return null
+ * values without actually evaluating it.
+ */
+ private boolean hasNullValues(AggregationFunction[] aggregationFunctions) {
+ for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ ExpressionContext argument = (ExpressionContext) aggregationFunction.getInputExpressions().get(0);
+ switch (argument.getType()) {
+ case IDENTIFIER:
+ DataSource dataSource = _indexSegment.getDataSource(argument.getIdentifier());
+ NullValueVectorReader nullValueVector = dataSource.getNullValueVector();
+ if (nullValueVector != null && !nullValueVector.getNullBitmap().isEmpty()) {
+ return true;
+ }
+ break;
+ case LITERAL:
+ return false;
Review Comment:
Yeah, that makes sense 👍
--
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