You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "wirybeaver (via GitHub)" <gi...@apache.org> on 2026/09/25 08:42:26 UTC
[PR] [core] Fix ANY_VALUE bytes intermediate round trip [pinot]
wirybeaver opened a new pull request, #19667:
URL: https://github.com/apache/pinot/pull/19667
## Summary
`ANY_VALUE` intermediate-state deserialization returns a raw `byte[]` for BYTES, but `extractFinalResult` expects a `Comparable`. A leaf-to-intermediate hop can therefore fail with `ClassCastException` even without disk spill. Deserialize BYTES into `ByteArray` and support reserializing that type, without changing the on-wire representation.
This is an independent, backportable fix extracted from the review of #19469. Please merge it before the aggregation-spill PR.
## Test Plan
- `./mvnw -pl pinot-core -Dtest=AnyValueAggregationFunctionTest test`: 37 tests passed, including a direct BYTES serialize/deserialize/final-result regression test and reserialization compatibility.
- `spotless:apply`, `checkstyle:check`, `license:format`, and `license:check` passed in `pinot-core`.
## Revert Plan
Revert the commit; this restores the previous BYTES intermediate representation and its existing failure mode.
## Issue Links
None - this is a regression identified during the review of #19469.
--
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] [core] Fix ANY_VALUE bytes intermediate round trip [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19667:
URL: https://github.com/apache/pinot/pull/19667#discussion_r4113683421
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AnyValueAggregationFunction.java:
##########
@@ -309,7 +312,7 @@ private Object deserializeValue(ByteBuffer buffer) {
case BIG_DECIMAL:
return new BigDecimal(new String(deserializeVariableBytes(buffer), StandardCharsets.UTF_8));
case BYTES:
- return deserializeVariableBytes(buffer);
+ return new ByteArray(deserializeVariableBytes(buffer));
Review Comment:
`ANY_VALUE` still stores a raw `byte[]` when it selects a BYTES value (`getDictionaryValue` / `getDirectValue`), but this deserializer now returns `ByteArray`. That makes the intermediate type depend on whether it crossed ser/de. It also leaves the normal BYTES DataTable writer's `(ByteArray) result` cast failing for values produced directly by aggregation. Could we wrap the selected value when storing it in the aggregation result holder, keep `ByteArray` on both sides of ser/de, and update this test to serialize an actual aggregation result and assert its type before and after? Once that invariant holds, the `instanceof byte[]` serialization branch can be removed.
--
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] [core] Fix ANY_VALUE bytes intermediate round trip [pinot]
Posted by "wirybeaver (via GitHub)" <gi...@apache.org>.
wirybeaver commented on code in PR #19667:
URL: https://github.com/apache/pinot/pull/19667#discussion_r4116416539
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AnyValueAggregationFunction.java:
##########
@@ -309,7 +312,7 @@ private Object deserializeValue(ByteBuffer buffer) {
case BIG_DECIMAL:
return new BigDecimal(new String(deserializeVariableBytes(buffer), StandardCharsets.UTF_8));
case BYTES:
- return deserializeVariableBytes(buffer);
+ return new ByteArray(deserializeVariableBytes(buffer));
Review Comment:
Fixed in dcf2f9f9180b5144d016980745d37f12dc6033f7. Dictionary-backed BYTES values now use getByteArrayValue, and direct BYTES values are wrapped before entering aggregation result holders, so both paths retain ByteArray before and after serialization. The regression test uses actual aggregate and SV/MV group-by results, checks final extraction and the round trip, and the obsolete byte[] serializer branch is removed without changing the wire format.
--
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] [core] Fix ANY_VALUE bytes intermediate round trip [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19667:
URL: https://github.com/apache/pinot/pull/19667#issuecomment-5830165363
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19667?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 ([`e6787e1`](https://app.codecov.io/gh/apache/pinot/commit/e6787e178645fd3c5a887e2aba7e0de8343cd524?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`910d0a3`](https://app.codecov.io/gh/apache/pinot/commit/910d0a391295bd82a468c6276cdf5a52b608e373?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 133 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19667 +/- ##
==============================================
+ Coverage 67.69% 100.00% +32.30%
+ Complexity 1430 6 -1424
==============================================
Files 3488 3 -3485
Lines 224306 6 -224300
Branches 35416 0 -35416
==============================================
- Hits 151850 6 -151844
+ Misses 60447 0 -60447
+ Partials 12009 0 -12009
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19667/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/19667/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/19667/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19667/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19667/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+32.30%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19667/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/19667/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/19667/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+32.30%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19667/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/19667/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/19667/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/19667?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] [core] Fix ANY_VALUE bytes intermediate round trip [pinot]
Posted by "wirybeaver (via GitHub)" <gi...@apache.org>.
wirybeaver commented on code in PR #19667:
URL: https://github.com/apache/pinot/pull/19667#discussion_r4116416539
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AnyValueAggregationFunction.java:
##########
@@ -309,7 +312,7 @@ private Object deserializeValue(ByteBuffer buffer) {
case BIG_DECIMAL:
return new BigDecimal(new String(deserializeVariableBytes(buffer), StandardCharsets.UTF_8));
case BYTES:
- return deserializeVariableBytes(buffer);
+ return new ByteArray(deserializeVariableBytes(buffer));
Review Comment:
Fixed in dcf2f9f9180b5144d016980745d37f12dc6033f7. Dictionary-backed BYTES values now use getByteArrayValue, and direct BYTES values are wrapped before entering aggregation result holders, so both paths retain ByteArray before and after serialization. The regression test uses actual aggregate and SV/MV group-by results, checks final extraction and the round trip, and the obsolete byte[] serializer branch is removed without changing the wire format.
[addressed by agent]
--
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] [core] Fix ANY_VALUE bytes intermediate round trip [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19667:
URL: https://github.com/apache/pinot/pull/19667
--
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