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/08/16 21:57:32 UTC
[GitHub] [pinot] shenyu0127 opened a new pull request, #11365: VarianceAggregationFunction NULL support.
shenyu0127 opened a new pull request, #11365:
URL: https://github.com/apache/pinot/pull/11365
Add NULL support for the following aggregation functions.
- STDDEVPOP
- STDDEVSAMP
- VARPOP
- VARSAMP
Fix a bug when all the values are NULL the function should return NULL (not `Double.NEGATIVE_INFINITY`).
--
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 #11365: VarianceAggregationFunction NULL support.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11365:
URL: https://github.com/apache/pinot/pull/11365#discussion_r1299318701
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/VarianceAggregationFunction.java:
##########
@@ -134,12 +169,7 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
@Override
public VarianceTuple extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
- VarianceTuple varianceTuple = aggregationResultHolder.getResult();
- if (varianceTuple == null) {
- return new VarianceTuple(0L, 0.0, 0.0);
- } else {
- return varianceTuple;
- }
+ return aggregationResultHolder.getResult();
Review Comment:
Do we need to check if `_nullHandlingEnabled` is set? Will it break if it is not set and this returns `null`?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/VarianceAggregationFunction.java:
##########
@@ -116,8 +139,20 @@ protected void setGroupByResult(int groupKey, GroupByResultHolder groupByResultH
public void aggregateGroupBySV(int length, int[] groupKeyArray, GroupByResultHolder groupByResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
double[] values = StatisticalAggregationFunctionUtils.getValSet(blockValSetMap, _expression);
- for (int i = 0; i < length; i++) {
- setGroupByResult(groupKeyArray[i], groupByResultHolder, 1L, values[i], 0.0);
+ RoaringBitmap nullBitmap = null;
+ if (_nullHandlingEnabled) {
+ nullBitmap = blockValSetMap.get(_expression).getNullBitmap();
+ }
+ if (nullBitmap != null && !nullBitmap.isEmpty()) {
+ for (int i = 0; i < length; i++) {
+ if (!nullBitmap.contains(i)) {
+ setGroupByResult(groupKeyArray[i], groupByResultHolder, 1L, values[i], 0.0);
+ }
+ }
+ } else {
+ for (int i = 0; i < length; i++) {
+ setGroupByResult(groupKeyArray[i], groupByResultHolder, 1L, values[i], 0.0);
+ }
Review Comment:
We need to also add similar logic to `aggregateGroupByMV`. We don't support MV as null right now, but we should support regular MV group key
--
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 #11365: VarianceAggregationFunction NULL support.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11365:
URL: https://github.com/apache/pinot/pull/11365
--
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 #11365: VarianceAggregationFunction NULL support.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11365:
URL: https://github.com/apache/pinot/pull/11365#issuecomment-1681361159
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11365?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11365](https://app.codecov.io/gh/apache/pinot/pull/11365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (950fcaa) into [master](https://app.codecov.io/gh/apache/pinot/commit/e4220ecea583ff439363fd505497a3ec3cb3e4b6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e4220ec) will **decrease** coverage by `47.26%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11365 +/- ##
=============================================
- Coverage 61.50% 14.24% -47.26%
+ Complexity 6512 494 -6018
=============================================
Files 2233 2233
Lines 120084 120102 +18
Branches 18223 18233 +10
=============================================
- Hits 73853 17112 -56741
- Misses 40828 101575 +60747
+ Partials 5403 1415 -3988
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11365/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/11365/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/11365/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11365/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/11365/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.24% <0.00%> (-47.11%)` | :arrow_down: |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11365/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.63% <0.00%> (-46.74%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11365/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.24% <0.00%> (-47.26%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11365/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/11365/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.63% <0.00%> (-0.01%)` | :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/11365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...gregation/function/AggregationFunctionFactory.java](https://app.codecov.io/gh/apache/pinot/pull/11365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BZ2dyZWdhdGlvbkZ1bmN0aW9uRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-81.36%)` | :arrow_down: |
| [...regation/function/VarianceAggregationFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9WYXJpYW5jZUFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-79.69%)` | :arrow_down: |
... and [1389 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11365/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 a diff in pull request #11365: VarianceAggregationFunction NULL support.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #11365:
URL: https://github.com/apache/pinot/pull/11365#discussion_r1299599246
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/VarianceAggregationFunction.java:
##########
@@ -116,8 +139,20 @@ protected void setGroupByResult(int groupKey, GroupByResultHolder groupByResultH
public void aggregateGroupBySV(int length, int[] groupKeyArray, GroupByResultHolder groupByResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
double[] values = StatisticalAggregationFunctionUtils.getValSet(blockValSetMap, _expression);
- for (int i = 0; i < length; i++) {
- setGroupByResult(groupKeyArray[i], groupByResultHolder, 1L, values[i], 0.0);
+ RoaringBitmap nullBitmap = null;
+ if (_nullHandlingEnabled) {
+ nullBitmap = blockValSetMap.get(_expression).getNullBitmap();
+ }
+ if (nullBitmap != null && !nullBitmap.isEmpty()) {
+ for (int i = 0; i < length; i++) {
+ if (!nullBitmap.contains(i)) {
+ setGroupByResult(groupKeyArray[i], groupByResultHolder, 1L, values[i], 0.0);
+ }
+ }
+ } else {
+ for (int i = 0; i < length; i++) {
+ setGroupByResult(groupKeyArray[i], groupByResultHolder, 1L, values[i], 0.0);
+ }
Review Comment:
Indeed. Done.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/VarianceAggregationFunction.java:
##########
@@ -134,12 +169,7 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
@Override
public VarianceTuple extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
- VarianceTuple varianceTuple = aggregationResultHolder.getResult();
- if (varianceTuple == null) {
- return new VarianceTuple(0L, 0.0, 0.0);
- } else {
- return varianceTuple;
- }
+ return aggregationResultHolder.getResult();
Review Comment:
We don't need to check if `_nullHandlingEnabled` is set here. This is because we want to return NULL (not `-infi`) if `_nullHandlingEnabled` is false and no matching row is found.
There was a bug in my previous commit: if `_nullHandlingEnabled` is false and no matching row is found, it would not return NULL. I fixed it by modifying other places.
--
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 #11365: VarianceAggregationFunction NULL support.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #11365:
URL: https://github.com/apache/pinot/pull/11365#discussion_r1299602323
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/VarianceAggregationFunction.java:
##########
@@ -134,12 +169,7 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
@Override
public VarianceTuple extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
- VarianceTuple varianceTuple = aggregationResultHolder.getResult();
- if (varianceTuple == null) {
- return new VarianceTuple(0L, 0.0, 0.0);
- } else {
- return varianceTuple;
- }
+ return aggregationResultHolder.getResult();
Review Comment:
We don't need to check if `_nullHandlingEnabled` is set here. This is because we want to return NULL (not `-infi`) if `_nullHandlingEnabled` is false and no matching row is found.
There was a bug in my previous commit: if `_nullHandlingEnabled` is false and no matching row is found, it would return NaN. I fixed it by modifying other places.
--
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 #11365: VarianceAggregationFunction NULL support.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #11365:
URL: https://github.com/apache/pinot/pull/11365#discussion_r1300570493
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/VarianceAggregationFunction.java:
##########
@@ -134,12 +169,7 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
@Override
public VarianceTuple extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
- VarianceTuple varianceTuple = aggregationResultHolder.getResult();
- if (varianceTuple == null) {
- return new VarianceTuple(0L, 0.0, 0.0);
- } else {
- return varianceTuple;
- }
+ return aggregationResultHolder.getResult();
Review Comment:
Done.
--
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 #11365: VarianceAggregationFunction NULL support.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11365:
URL: https://github.com/apache/pinot/pull/11365#discussion_r1300511610
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/VarianceAggregationFunction.java:
##########
@@ -134,12 +169,7 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
@Override
public VarianceTuple extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
- VarianceTuple varianceTuple = aggregationResultHolder.getResult();
- if (varianceTuple == null) {
- return new VarianceTuple(0L, 0.0, 0.0);
- } else {
- return varianceTuple;
- }
+ return aggregationResultHolder.getResult();
Review Comment:
When null handling is disabled, we should not return `null` in the final result though. We should apply the same logic as in `AvgAggregationFunction`
--
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