You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2025/08/07 23:18:44 UTC
[PR] Enhance ValueAggregator [pinot]
Jackie-Jiang opened a new pull request, #16552:
URL: https://github.com/apache/pinot/pull/16552
- Handle `null` raw value in `ValueAggregator`. Ingestion aggregation might introduce `null` raw value when the column is not specified in the schema. (See more details in #16317)
- Introduce a systematic way to determine whether the aggregated value is of fixed size. Ingestion aggregation can be enabled only when the aggregated value is of fixed size.
- This also fix the bug where several supported aggregations are not allowed in ingestion aggregation: `AVG`, `MIN_MAX_RANGE`, `DISTINCT_COUNT_ULL`, `DISTINCT_COUNT_TUPLE_SKETCH`, `DISTINCT_COUNT_CPC_SKETCH`
--
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] Enhance ValueAggregator [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #16552:
URL: https://github.com/apache/pinot/pull/16552
--
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] Enhance ValueAggregator [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16552:
URL: https://github.com/apache/pinot/pull/16552#issuecomment-3169503812
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16552?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `11.86441%` with `104 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.30%. Comparing base ([`ff217a1`](https://app.codecov.io/gh/apache/pinot/commit/ff217a1a1bb68c75de7ae8ca9cd8a7b6b4d813c8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`494e515`](https://app.codecov.io/gh/apache/pinot/commit/494e5150fbb69d9bb7868a1efe54cc9f5c1d0c0f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16552?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Findexsegment%2Fmutable%2FMutableSegmentImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | 1.88% | [51 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Futils%2FTableConfigUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | 0.00% | [6 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/segment/local/aggregator/AvgValueAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Faggregator%2FAvgValueAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9hZ2dyZWdhdG9yL0F2Z1ZhbHVlQWdncmVnYXRvci5qYXZh) | 0.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...regator/DistinctCountCPCSketchValueAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Faggregator%2FDistinctCountCPCSketchValueAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9hZ2dyZWdhdG9yL0Rpc3RpbmN0Q291bnRDUENTa2V0Y2hWYWx1ZUFnZ3JlZ2F0b3IuamF2YQ==) | 0.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ggregator/DistinctCountHLLPlusValueAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Faggregator%2FDistinctCountHLLPlusValueAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9hZ2dyZWdhdG9yL0Rpc3RpbmN0Q291bnRITExQbHVzVmFsdWVBZ2dyZWdhdG9yLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...al/aggregator/DistinctCountHLLValueAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Faggregator%2FDistinctCountHLLValueAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9hZ2dyZWdhdG9yL0Rpc3RpbmN0Q291bnRITExWYWx1ZUFnZ3JlZ2F0b3IuamF2YQ==) | 0.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...al/aggregator/DistinctCountULLValueAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Faggregator%2FDistinctCountULLValueAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9hZ2dyZWdhdG9yL0Rpc3RpbmN0Q291bnRVTExWYWx1ZUFnZ3JlZ2F0b3IuamF2YQ==) | 0.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../aggregator/IntegerTupleSketchValueAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Faggregator%2FIntegerTupleSketchValueAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9hZ2dyZWdhdG9yL0ludGVnZXJUdXBsZVNrZXRjaFZhbHVlQWdncmVnYXRvci5qYXZh) | 25.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/segment/local/aggregator/MaxValueAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Faggregator%2FMaxValueAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9hZ2dyZWdhdG9yL01heFZhbHVlQWdncmVnYXRvci5qYXZh) | 0.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/local/aggregator/MinMaxRangeValueAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Faggregator%2FMinMaxRangeValueAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9hZ2dyZWdhdG9yL01pbk1heFJhbmdlVmFsdWVBZ2dyZWdhdG9yLmphdmE=) | 0.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [9 more](https://app.codecov.io/gh/apache/pinot/pull/16552?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
> :exclamation: There is a different number of reports uploaded between BASE (ff217a1) and HEAD (494e515). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (ff217a1) | HEAD (494e515) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|unittests2|2|0|
>|java-11|5|0|
>|integration|6|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16552 +/- ##
============================================
- Coverage 63.26% 56.30% -6.96%
+ Complexity 1362 699 -663
============================================
Files 3012 2356 -656
Lines 174417 129681 -44736
Branches 26708 20629 -6079
============================================
- Hits 110341 73017 -37324
+ Misses 55660 50621 -5039
+ Partials 8416 6043 -2373
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16552/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/16552/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/16552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16552/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/16552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16552/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/16552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.30% <11.86%> (-6.94%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.30% <11.86%> (-6.96%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.30% <11.86%> (-6.96%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.30% <11.86%> (-0.10%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16552/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/16552?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).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] Enhance ValueAggregator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2263811970
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -124,6 +126,8 @@ private TableConfigUtils() {
// this is duplicate with KinesisConfig.STREAM_TYPE, while instead of use KinesisConfig.STREAM_TYPE directly, we
// hardcode the value here to avoid pulling the entire pinot-kinesis module as dependency.
private static final String KINESIS_STREAM_TYPE = "kinesis";
+
+ @Deprecated(since = "1.5.0", forRemoval = true)
Review Comment:
I'm not sure if any client/third party code is using it, but sure I think I can remove it since the behavior is wrong
--
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] Enhance ValueAggregator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2264078860
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/CountValueAggregator.java:
##########
@@ -36,7 +37,7 @@ public DataType getAggregatedValueType() {
}
@Override
- public Long getInitialAggregatedValue(Object rawValue) {
+ public Long getInitialAggregatedValue(@Nullable Object rawValue) {
Review Comment:
Decided to go with correct SQL semantic, where `null` is not counted if the source is a column
--
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] Enhance ValueAggregator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2263813158
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -510,6 +512,18 @@ public static void validateIngestionConfig(TableConfig tableConfig, Schema schem
Preconditions.checkState(firstArgument.getType() == ExpressionContext.Type.IDENTIFIER,
"First argument of aggregation function: %s must be identifier, got: %s", functionType,
firstArgument.getType());
+ // Create a ValueAggregator for the aggregation function and check if it is supported for ingestion (fixed
+ // size aggregated value).
+ ValueAggregator<?, ?> valueAggregator;
+ try {
+ valueAggregator =
+ ValueAggregatorFactory.getValueAggregator(functionType, arguments.subList(1, numArguments));
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ "Caught exception while creating ValueAggregator for aggregation function: " + aggregationFunction, e);
+ }
+ Preconditions.checkState(valueAggregator.isAggregatedValueFixedSize(),
+ "Aggregation function: %s must have fixed size aggregated value", aggregationFunction);
Review Comment:
For ingestion aggregation, the same value is updated in-place, so we cannot handle size change
--
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] Enhance ValueAggregator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2263809098
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/CountValueAggregator.java:
##########
@@ -36,7 +37,7 @@ public DataType getAggregatedValueType() {
}
@Override
- public Long getInitialAggregatedValue(Object rawValue) {
+ public Long getInitialAggregatedValue(@Nullable Object rawValue) {
Review Comment:
I'm debating on that. The correct SQL semantic should not count `null`, but that is a behavior change
--
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] Enhance ValueAggregator [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2261629541
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/MinMaxRangeValueAggregator.java:
##########
@@ -38,7 +39,10 @@ public DataType getAggregatedValueType() {
}
@Override
- public MinMaxRangePair getInitialAggregatedValue(Object rawValue) {
+ public MinMaxRangePair getInitialAggregatedValue(@Nullable Object rawValue) {
+ if (rawValue == null) {
+ return new MinMaxRangePair();
Review Comment:
Creating a MinMaxRangePair with no arguments when rawValue is null may not represent the correct initial state. The constructor should likely initialize with appropriate default values (e.g., Double.POSITIVE_INFINITY for min, Double.NEGATIVE_INFINITY for max) to ensure proper aggregation behavior.
```suggestion
return new MinMaxRangePair(Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY);
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/AvgValueAggregator.java:
##########
@@ -38,7 +39,10 @@ public DataType getAggregatedValueType() {
}
@Override
- public AvgPair getInitialAggregatedValue(Object rawValue) {
+ public AvgPair getInitialAggregatedValue(@Nullable Object rawValue) {
+ if (rawValue == null) {
+ return new AvgPair();
Review Comment:
Creating an AvgPair with no arguments when rawValue is null may not represent the correct initial state for average calculations. The constructor should initialize with appropriate values (likely sum=0, count=0) to ensure correct averaging behavior when subsequent values are aggregated.
```suggestion
return new AvgPair(0.0, 0L);
```
--
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] Enhance ValueAggregator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2263816253
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -1011,37 +1016,40 @@ private void recordIndexingError(String indexType) {
private void aggregateMetrics(GenericRow row, int docId) {
for (MetricFieldSpec metricFieldSpec : _physicalMetricFieldSpecs) {
IndexContainer indexContainer = _indexContainerMap.get(metricFieldSpec.getName());
+ // NOTE: value can be null if the column is not specified in the schema.
Object value = row.getValue(indexContainer._sourceColumn);
+ // Skip aggregation if the input value is null.
+ if (value == null) {
+ continue;
+ }
MutableForwardIndex forwardIndex =
(MutableForwardIndex) indexContainer._mutableIndexes.get(StandardIndexes.forward());
DataType dataType = metricFieldSpec.getDataType();
- Double oldDoubleValue;
- Double newDoubleValue;
- Long oldLongValue;
- Long newLongValue;
ValueAggregator valueAggregator = indexContainer._valueAggregator;
switch (valueAggregator.getAggregatedValueType()) {
case DOUBLE:
+ double oldDoubleValue;
+ double newDoubleValue;
switch (dataType) {
case INT:
oldDoubleValue = ((Integer) forwardIndex.getInt(docId)).doubleValue();
- newDoubleValue = (Double) valueAggregator.applyRawValue(oldDoubleValue, value);
- forwardIndex.setInt(docId, newDoubleValue.intValue());
+ newDoubleValue = (double) valueAggregator.applyRawValue(oldDoubleValue, value);
Review Comment:
Good point. Didn't realize this is extra boxing/unboxing
--
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] Enhance ValueAggregator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2264078230
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/CountValueAggregator.java:
##########
@@ -36,7 +37,7 @@ public DataType getAggregatedValueType() {
}
@Override
- public Long getInitialAggregatedValue(Object rawValue) {
+ public Long getInitialAggregatedValue(@Nullable Object rawValue) {
return 1L;
Review Comment:
Fixed to be consistent
--
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] Enhance ValueAggregator [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2262350520
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/CountValueAggregator.java:
##########
@@ -36,7 +37,7 @@ public DataType getAggregatedValueType() {
}
@Override
- public Long getInitialAggregatedValue(Object rawValue) {
+ public Long getInitialAggregatedValue(@Nullable Object rawValue) {
Review Comment:
Should we be counting `null` values here?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -124,6 +126,8 @@ private TableConfigUtils() {
// this is duplicate with KinesisConfig.STREAM_TYPE, while instead of use KinesisConfig.STREAM_TYPE directly, we
// hardcode the value here to avoid pulling the entire pinot-kinesis module as dependency.
private static final String KINESIS_STREAM_TYPE = "kinesis";
+
+ @Deprecated(since = "1.5.0", forRemoval = true)
Review Comment:
This isn't in the `spi` module so why can't we just directly remove it?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/ValueAggregator.java:
##########
@@ -42,8 +43,10 @@ public interface ValueAggregator<R, A> {
/**
* Returns the initial aggregated value.
+ * <p>NOTE: rawValue can be null when the aggregator is used for ingestion aggregation, and the column is not
+ * specified in the schema.
Review Comment:
Is this true even if null handling is enabled and we get actual `null` values? Will those values always be substituted with the default null value for the column?
--
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] Enhance ValueAggregator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2263810345
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/ValueAggregator.java:
##########
@@ -42,8 +43,10 @@ public interface ValueAggregator<R, A> {
/**
* Returns the initial aggregated value.
+ * <p>NOTE: rawValue can be null when the aggregator is used for ingestion aggregation, and the column is not
+ * specified in the schema.
Review Comment:
We get `null` values with/without null handling because the source column is not specified in the schema, and we don't know what is the default null value (or data type)
--
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] Enhance ValueAggregator [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on code in PR #16552:
URL: https://github.com/apache/pinot/pull/16552#discussion_r2262540224
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/CountValueAggregator.java:
##########
@@ -36,7 +37,7 @@ public DataType getAggregatedValueType() {
}
@Override
- public Long getInitialAggregatedValue(Object rawValue) {
+ public Long getInitialAggregatedValue(@Nullable Object rawValue) {
return 1L;
Review Comment:
For AvgValueAggregator, We do
```
if (rawValue == null) {
return new AvgPair(0, 0);
}
```
In above, sum and count is 0 for null object.
But for this CountValueAggregator, we have a different behaviour for null objects.
Is above correct?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -510,6 +512,18 @@ public static void validateIngestionConfig(TableConfig tableConfig, Schema schem
Preconditions.checkState(firstArgument.getType() == ExpressionContext.Type.IDENTIFIER,
"First argument of aggregation function: %s must be identifier, got: %s", functionType,
firstArgument.getType());
+ // Create a ValueAggregator for the aggregation function and check if it is supported for ingestion (fixed
+ // size aggregated value).
+ ValueAggregator<?, ?> valueAggregator;
+ try {
+ valueAggregator =
+ ValueAggregatorFactory.getValueAggregator(functionType, arguments.subList(1, numArguments));
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ "Caught exception while creating ValueAggregator for aggregation function: " + aggregationFunction, e);
+ }
+ Preconditions.checkState(valueAggregator.isAggregatedValueFixedSize(),
+ "Aggregation function: %s must have fixed size aggregated value", aggregationFunction);
Review Comment:
Why only fixed size is supported?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -1011,37 +1016,40 @@ private void recordIndexingError(String indexType) {
private void aggregateMetrics(GenericRow row, int docId) {
for (MetricFieldSpec metricFieldSpec : _physicalMetricFieldSpecs) {
IndexContainer indexContainer = _indexContainerMap.get(metricFieldSpec.getName());
+ // NOTE: value can be null if the column is not specified in the schema.
Object value = row.getValue(indexContainer._sourceColumn);
+ // Skip aggregation if the input value is null.
+ if (value == null) {
+ continue;
+ }
MutableForwardIndex forwardIndex =
(MutableForwardIndex) indexContainer._mutableIndexes.get(StandardIndexes.forward());
DataType dataType = metricFieldSpec.getDataType();
- Double oldDoubleValue;
- Double newDoubleValue;
- Long oldLongValue;
- Long newLongValue;
ValueAggregator valueAggregator = indexContainer._valueAggregator;
switch (valueAggregator.getAggregatedValueType()) {
case DOUBLE:
+ double oldDoubleValue;
+ double newDoubleValue;
switch (dataType) {
case INT:
oldDoubleValue = ((Integer) forwardIndex.getInt(docId)).doubleValue();
- newDoubleValue = (Double) valueAggregator.applyRawValue(oldDoubleValue, value);
- forwardIndex.setInt(docId, newDoubleValue.intValue());
+ newDoubleValue = (double) valueAggregator.applyRawValue(oldDoubleValue, value);
Review Comment:
Do we want to do similar for `oldDoubleValue` and `oldLongValue` ?
--
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