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 2026/08/11 21:27:52 UTC
[PR] Give the integer tuple sketch functions the query's null handling option [pinot]
Jackie-Jiang opened a new pull request, #19217:
URL: https://github.com/apache/pinot/pull/19217
## Summary
The integer tuple sketch family never received the query's null handling option. Its constructors did not take it, so a null row's column default was folded into the union whatever the query asked for, and `extractFinalResult` could not tell the two modes apart when rendering an empty input.
Second of the follow-ups recorded as known deviation 1 on `AggregationFunction` in #19158, after #19211.
### Behavior change
With `enableNullHandling=true`, `DISTINCTCOUNTTUPLESKETCH`, `DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH`, `SUMVALUESINTEGERSUMTUPLESKETCH` and `AVGVALUEINTEGERSUMTUPLESKETCH` now skip rows that carry no sketch.
**On the multi-stage engine the change is unconditional, and no query option controls it.** `AggregateOperator` builds every aggregation function with null handling enabled and never consults the query's option, so these functions previously discarded a flag the engine had already set to `true`. Worth a release note on the same footing as the multi-value functions in #19158.
With the option disabled, each function still renders exactly what it rendered before.
### Only the rows that carry a sketch are deserialized
This family's input is always a `BYTES` column of serialized sketches. The old code deserialized every row up front with `deserializeSketches(values, length)` and then unioned them, so a null row was heapified from the column default before it could be skipped. It is now a per-row `deserializeSketch`, called only inside the non-null ranges.
The base is re-parented from `BaseSingleInputAggregationFunction` onto `NullableSingleInputAggregationFunction`, which extends it, so this is a compatible swap that supplies both the option and `forEachNotNull`.
### The accumulator is created inside the range
`getAccumulator` eagerly created *and stored* an accumulator in the result holder before the aggregation loop. Left that way, a block of nulls would store an empty accumulator and the "nothing was aggregated" signal would never reach `extractFinalResult`. It is now created inside the non-null range, so an untouched holder stays untouched.
### Each function renders its own identity when the option is off
The four functions do not share an empty answer, so each preserves its own:
| function | disabled-mode value | why |
|---|---|---|
| `DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH` | base64 of an empty sketch | built through a new `emptyAccumulator()` where the value is rendered |
| `DISTINCTCOUNTTUPLESKETCH` | `0` | an empty sketch estimates to zero |
| `SUMVALUESINTEGERSUMTUPLESKETCH` | `0` | an empty sketch sums to `0 / theta` |
| `AVGVALUEINTEGERSUMTUPLESKETCH` | `NULL` | already `NULL` in both modes via the retained-entries check |
The raw variant is the reason the identity is constructed at the render point rather than substituted during extraction: substituting upstream is what destroys the signal, and returning `null` there would silently turn a rendered value into `NULL`.
### Tests
`AggregationFunctionNullContractTest` **cannot reach these functions**. Its synthetic `BYTES` block supplies empty byte arrays, which are not deserializable sketches, so all four are pinned in its skip list. The suite passes unchanged here, and that is not evidence about this change.
`IntegerTupleSketchNullHandlingTest` is therefore the only coverage, and says so in its Javadoc. It builds real serialized sketches and covers null rows being skipped, an all-null input producing no intermediate result, and the per-function answers in both modes.
The untouched-holder behaviour is mutation-tested: hoisting the accumulator creation back out of the range fails `testEveryRowNullYieldsNoIntermediateResult`.
--
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] Give the integer tuple sketch functions the query's null handling option [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19217:
URL: https://github.com/apache/pinot/pull/19217
--
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] Give the integer tuple sketch functions the query's null handling option [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19217:
URL: https://github.com/apache/pinot/pull/19217#discussion_r3762480823
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/IntegerTupleSketchAggregationFunction.java:
##########
@@ -156,11 +157,14 @@ public void aggregate(int length, AggregationResultHolder aggregationResultHolde
if (storedType == FieldSpec.DataType.BYTES) {
byte[][] bytesValues = blockValSet.getBytesValuesSV();
try {
- TupleIntSketchAccumulator tupleIntSketchAccumulator = getAccumulator(aggregationResultHolder);
- TupleSketch<IntegerSummary>[] sketches = deserializeSketches(bytesValues, length);
- for (TupleSketch<IntegerSummary> sketch : sketches) {
- tupleIntSketchAccumulator.apply(sketch);
- }
+ // the accumulator is created inside the range, so an all-null block leaves the holder untouched and
+ // extractFinalResult sees the null that means nothing was aggregated
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ TupleIntSketchAccumulator tupleIntSketchAccumulator = getAccumulator(aggregationResultHolder);
+ for (int i = from; i < to; i++) {
+ tupleIntSketchAccumulator.apply(deserializeSketch(bytesValues[i]));
+ }
Review Comment:
Correct, and it was a real hole. `forEachNotNull` calls the consumer with `(0, 0)` for a zero-length block, so creating the accumulator inside the range was not enough — the range itself can be empty, and the holder was marked as aggregated for an input that aggregated nothing.
The range now returns early when `to == from`, before the accumulator exists.
`testZeroLengthBlockLeavesTheHolderUntouched` pins it, and I mutation-tested the guard: removing it fails that test. Nothing else covered the case, so thank you — this would have shipped.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/IntegerTupleSketchAggregationFunction.java:
##########
@@ -208,12 +211,13 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
if (singleValue && storedType == FieldSpec.DataType.BYTES) {
byte[][] bytesValues = blockValSetMap.get(_expression).getBytesValuesSV();
try {
- TupleSketch<IntegerSummary>[] sketches = deserializeSketches(bytesValues, length);
- for (int i = 0; i < length; i++) {
- for (int groupKey : groupKeysArray[i]) {
- getAccumulator(groupByResultHolder, groupKey).apply(sketches[i]);
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ for (int groupKey : groupKeysArray[i]) {
+ getAccumulator(groupByResultHolder, groupKey).apply(deserializeSketch(bytesValues[i]));
+ }
}
Review Comment:
Correct, and a regression I introduced. The previous code deserialized each row once into an array before the loop; moving deserialization into the range put it inside the group-key loop, so a row belonging to N group keys was heapified N times.
Now deserialized once per row, above the inner loop, and reused for every group key that row belongs to. The single-value group-by path was already once per row and is unchanged.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java:
##########
@@ -481,13 +481,16 @@ public static AggregationFunction getAggregationFunction(FunctionContext functio
return new FourthMomentAggregationFunction(arguments, FourthMomentAggregationFunction.Type.MOMENT);
case DISTINCTCOUNTTUPLESKETCH:
// mode actually doesn't matter here because we only care about keys, not values
- return new DistinctCountIntegerTupleSketchAggregationFunction(arguments, IntegerSummary.Mode.Sum);
+ return new DistinctCountIntegerTupleSketchAggregationFunction(arguments, IntegerSummary.Mode.Sum,
Review Comment:
Agreed, and it is the honest gap in this PR. The coverage here is direct construction, so it does not prove the factory plumbing reaches server aggregation or that the rendered value survives to the broker.
I have not added end-to-end cases, because the existing tuple sketch fixtures carry no null `BYTES` rows and building that fixture is a larger piece than this change. Tracking it on #19218 rather than claiming coverage this PR does not have — the description now says the unit test is the only coverage and why the contract test cannot reach this family.
--
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] Give the integer tuple sketch functions the query's null handling option [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19217:
URL: https://github.com/apache/pinot/pull/19217#issuecomment-5270116172
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/979 (merged).
--
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] Give the integer tuple sketch functions the query's null handling option [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #19217:
URL: https://github.com/apache/pinot/pull/19217#issuecomment-5260068186
@yashmayya thanks for the review — both notes were right, and both are fixed.
**The stale doc line.** You are right about which comment attached: the old `/// Deserializes the sketches from the bytes.` sat above the annotation and my accurate line went below it, so the stale one was the Javadoc and mine was an ordinary comment. I replaced the method and left its old documentation in place. The stale line is gone and mine is above the signature.
`@SuppressWarnings({"unchecked"})` is gone too — it covered the `new TupleSketch[length]` generic array creation, which the per-row deserializer no longer does. Good catch that it had become dead.
**The group-by disabled-mode answer.** Also right, and the description was wrong to say the disabled mode renders exactly what it rendered before. It does on the aggregation path; on the group-by path a group holding no rows — which a filtered aggregation creates — moves `SUMVALUESINTEGERSUMTUPLESKETCH` from `NULL` to `0` and the raw variant from `NULL` to the serialized empty sketch. The description now states this, and that it resolves the group-versus-no-group inconsistency in favour of the aggregation path's answer.
**Two more from the Copilot review, both real.** A zero-length block still reaches the range callback as `(0, 0)`, so the accumulator was being created for an input that aggregated nothing; the range now returns early and a new test pins it. And moving deserialization into the range accidentally put it inside the group-key loop on the multi-value path, heapifying a row once per group key instead of once — now hoisted back out.
The mixed-version note is in the description as well: a new server sends `null` where it used to send an empty accumulator, an old broker renders that as `NULL`, and upgrading brokers first avoids it.
--
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] Give the integer tuple sketch functions the query's null handling option [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19217:
URL: https://github.com/apache/pinot/pull/19217#issuecomment-5259465811
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19217?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`e85af3c`](https://app.codecov.io/gh/apache/pinot/commit/e85af3cad87182bcd49c52b781cfd76b9d764ec4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5b02681`](https://app.codecov.io/gh/apache/pinot/commit/5b0268158ba9e80765aee5ed6c435e823e25d5da?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19217 +/- ##
==============================================
+ Coverage 66.94% 100.00% +33.05%
+ Complexity 1423 6 -1417
==============================================
Files 3452 3 -3449
Lines 218515 6 -218509
Branches 34739 0 -34739
==============================================
- Hits 146281 6 -146275
+ Misses 60549 0 -60549
+ Partials 11685 0 -11685
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19217/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+33.05%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+33.05%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19217/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/19217/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/19217/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 Harness](https://app.codecov.io/gh/apache/pinot/pull/19217?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] Give the integer tuple sketch functions the query's null handling option [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19217:
URL: https://github.com/apache/pinot/pull/19217#discussion_r3761886806
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/IntegerTupleSketchAggregationFunction.java:
##########
@@ -156,11 +157,14 @@ public void aggregate(int length, AggregationResultHolder aggregationResultHolde
if (storedType == FieldSpec.DataType.BYTES) {
byte[][] bytesValues = blockValSet.getBytesValuesSV();
try {
- TupleIntSketchAccumulator tupleIntSketchAccumulator = getAccumulator(aggregationResultHolder);
- TupleSketch<IntegerSummary>[] sketches = deserializeSketches(bytesValues, length);
- for (TupleSketch<IntegerSummary> sketch : sketches) {
- tupleIntSketchAccumulator.apply(sketch);
- }
+ // the accumulator is created inside the range, so an all-null block leaves the holder untouched and
+ // extractFinalResult sees the null that means nothing was aggregated
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ TupleIntSketchAccumulator tupleIntSketchAccumulator = getAccumulator(aggregationResultHolder);
+ for (int i = from; i < to; i++) {
+ tupleIntSketchAccumulator.apply(deserializeSketch(bytesValues[i]));
+ }
Review Comment:
`forEachNotNull` still invokes the callback with `(0, 0)` when a zero-length block has no null bitmap. Creating the accumulator before verifying the range is non-empty therefore marks an empty input as aggregated: with null handling enabled, the raw function emits a serialized empty sketch and `SUMVALUES...` emits `0` instead of `NULL`. Keep the holder untouched for an empty range.
This issue also appears on line 233 of the same file.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/IntegerTupleSketchAggregationFunction.java:
##########
@@ -208,12 +211,13 @@ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResult
if (singleValue && storedType == FieldSpec.DataType.BYTES) {
byte[][] bytesValues = blockValSetMap.get(_expression).getBytesValuesSV();
try {
- TupleSketch<IntegerSummary>[] sketches = deserializeSketches(bytesValues, length);
- for (int i = 0; i < length; i++) {
- for (int groupKey : groupKeysArray[i]) {
- getAccumulator(groupByResultHolder, groupKey).apply(sketches[i]);
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ for (int groupKey : groupKeysArray[i]) {
+ getAccumulator(groupByResultHolder, groupKey).apply(deserializeSketch(bytesValues[i]));
+ }
}
Review Comment:
`deserializeSketch` is now inside the group-key loop, so a row assigned to N group keys heapifies the same serialized sketch N times. This regresses the previous once-per-row behavior on a query hot path; deserialize once before the inner loop and reuse the sketch.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java:
##########
@@ -481,13 +481,16 @@ public static AggregationFunction getAggregationFunction(FunctionContext functio
return new FourthMomentAggregationFunction(arguments, FourthMomentAggregationFunction.Type.MOMENT);
case DISTINCTCOUNTTUPLESKETCH:
// mode actually doesn't matter here because we only care about keys, not values
- return new DistinctCountIntegerTupleSketchAggregationFunction(arguments, IntegerSummary.Mode.Sum);
+ return new DistinctCountIntegerTupleSketchAggregationFunction(arguments, IntegerSummary.Mode.Sum,
Review Comment:
The new unit test directly instantiates these functions, so it does not verify that this factory plumbing reaches server aggregation and broker rendering. The existing `TupleSketchTest` integration data contains no null BYTES values. Add end-to-end cases for both query engines with nullable sketch rows, covering enabled-mode skipping/all-null answers and disabled-mode legacy identities.
--
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