You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/01/29 01:34:45 UTC
[PR] Fix multi-stage query stats serde compatibility issue caused by UNNEST [pinot]
yashmayya opened a new pull request, #17590:
URL: https://github.com/apache/pinot/pull/17590
- Fixes backward compatibility failures like https://github.com/apache/pinot/actions/runs/21444370350/job/61755861198?pr=17552:
```
"exceptions":[{"errorCode":1000,"message":"Received 1 error: Cannot merge stats from different stages. Found types [MAILBOX_RECEIVE, MAILBOX_RECEIVE, WINDOW, AGGREGATE, MAILBOX_SEND] and [MAILBOX_RECEIVE, MAILBOX_RECEIVE, UNION, AGGREGATE, MAILBOX_SEND]\njava.lang.IllegalStateException: Cannot merge stats from different stages. Found types [MAILBOX_RECEIVE, MAILBOX_RECEIVE, WINDOW, AGGREGATE, MAILBOX_SEND] and [MAILBOX_RECEIVE, MAILBOX_RECEIVE, UNION, AGGREGATE, MAILBOX_SEND]
```
- Update the enum to make future breakages more unlikely by introducing an explicit serial ID.
--
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] Fix multi-stage query stats serde compatibility issue caused by UNNEST [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17590:
URL: https://github.com/apache/pinot/pull/17590#discussion_r2739518249
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -426,21 +423,67 @@ public void updateServerMetrics(StatMap<?> map, ServerMetrics serverMetrics) {
}
}
},
- LOOKUP_JOIN(LookupJoinOperator.StatKey.class) {
+ LOOKUP_JOIN(14, LookupJoinOperator.StatKey.class) {
@Override
public void mergeInto(BrokerResponseNativeV2 response, StatMap<?> map) {
@SuppressWarnings("unchecked")
StatMap<LookupJoinOperator.StatKey> stats = (StatMap<LookupJoinOperator.StatKey>) map;
response.mergeMaxRowsInOperator(stats.getLong(LookupJoinOperator.StatKey.EMITTED_ROWS));
}
+ },
+ UNNEST(15, UnnestOperator.StatKey.class) {
+ @Override
+ public void mergeInto(BrokerResponseNativeV2 response, StatMap<?> map) {
+ @SuppressWarnings("unchecked")
+ StatMap<UnnestOperator.StatKey> stats = (StatMap<UnnestOperator.StatKey>) map;
+ response.mergeMaxRowsInOperator(stats.getLong(UnnestOperator.StatKey.EMITTED_ROWS));
+ }
};
+ private static final Map<Integer, Type> ID_TO_TYPE;
Review Comment:
Consider making it a `Type[]` for faster lookup. We can make it similar to `DataTable.MetadataKey`
--
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] Fix multi-stage query stats serde compatibility issue caused by UNNEST [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17590:
URL: https://github.com/apache/pinot/pull/17590#issuecomment-3815047280
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17590?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 `75.00000%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.49%. Comparing base ([`b4081d6`](https://app.codecov.io/gh/apache/pinot/commit/b4081d6003347020cc5e38eb3c60638c6aa8f1de?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`dbde112`](https://app.codecov.io/gh/apache/pinot/commit/dbde1122bd23a28e3df287fe10802a1627738037?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17590?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...not/query/runtime/operator/MultiStageOperator.java](https://app.codecov.io/gh/apache/pinot/pull/17590?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FMultiStageOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aVN0YWdlT3BlcmF0b3IuamF2YQ==) | 78.78% | [5 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17590?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pinot/query/runtime/plan/MultiStageQueryStats.java](https://app.codecov.io/gh/apache/pinot/pull/17590?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fplan%2FMultiStageQueryStats.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL011bHRpU3RhZ2VRdWVyeVN0YXRzLmphdmE=) | 57.14% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17590?src=pr&el=tree&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 (b4081d6) and HEAD (dbde112). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (b4081d6) | HEAD (dbde112) |
>|------|------|------|
>|java-21|5|0|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|1|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17590 +/- ##
============================================
- Coverage 63.17% 55.49% -7.68%
+ Complexity 1479 700 -779
============================================
Files 3173 2474 -699
Lines 189865 140067 -49798
Branches 29057 22331 -6726
============================================
- Hits 119946 77733 -42213
+ Misses 60587 55765 -4822
+ Partials 9332 6569 -2763
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17590/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/17590/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/17590/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/17590/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/17590/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/17590/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <75.00%> (-7.65%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17590/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17590/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <75.00%> (-7.68%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17590/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <75.00%> (-7.68%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17590/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <75.00%> (-0.02%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17590/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/17590?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] Fix multi-stage query stats serde compatibility issue caused by UNNEST [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17590:
URL: https://github.com/apache/pinot/pull/17590#discussion_r2739502368
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -426,21 +423,67 @@ public void updateServerMetrics(StatMap<?> map, ServerMetrics serverMetrics) {
}
}
},
- LOOKUP_JOIN(LookupJoinOperator.StatKey.class) {
+ LOOKUP_JOIN(14, LookupJoinOperator.StatKey.class) {
@Override
public void mergeInto(BrokerResponseNativeV2 response, StatMap<?> map) {
@SuppressWarnings("unchecked")
StatMap<LookupJoinOperator.StatKey> stats = (StatMap<LookupJoinOperator.StatKey>) map;
response.mergeMaxRowsInOperator(stats.getLong(LookupJoinOperator.StatKey.EMITTED_ROWS));
}
+ },
+ UNNEST(15, UnnestOperator.StatKey.class) {
+ @Override
+ public void mergeInto(BrokerResponseNativeV2 response, StatMap<?> map) {
+ @SuppressWarnings("unchecked")
+ StatMap<UnnestOperator.StatKey> stats = (StatMap<UnnestOperator.StatKey>) map;
+ response.mergeMaxRowsInOperator(stats.getLong(UnnestOperator.StatKey.EMITTED_ROWS));
+ }
};
+ private static final Map<Integer, Type> ID_TO_TYPE;
+
+ static {
+ Map<Integer, Type> map = new HashMap<>();
+ for (Type type : values()) {
+ int id = type._id;
+ if (id < 0 || id > Byte.MAX_VALUE) {
Review Comment:
The validation allows `id == Byte.MAX_VALUE` (127), but since the ID is written using `output.writeByte()` and compared as a signed byte, negative values are possible when reading back. Values from 128-255 would be interpreted as negative when read. The upper bound should be 127 to match the signed byte range used in serialization.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -426,21 +423,67 @@ public void updateServerMetrics(StatMap<?> map, ServerMetrics serverMetrics) {
}
}
},
- LOOKUP_JOIN(LookupJoinOperator.StatKey.class) {
+ LOOKUP_JOIN(14, LookupJoinOperator.StatKey.class) {
@Override
public void mergeInto(BrokerResponseNativeV2 response, StatMap<?> map) {
@SuppressWarnings("unchecked")
StatMap<LookupJoinOperator.StatKey> stats = (StatMap<LookupJoinOperator.StatKey>) map;
response.mergeMaxRowsInOperator(stats.getLong(LookupJoinOperator.StatKey.EMITTED_ROWS));
}
+ },
+ UNNEST(15, UnnestOperator.StatKey.class) {
+ @Override
+ public void mergeInto(BrokerResponseNativeV2 response, StatMap<?> map) {
+ @SuppressWarnings("unchecked")
+ StatMap<UnnestOperator.StatKey> stats = (StatMap<UnnestOperator.StatKey>) map;
+ response.mergeMaxRowsInOperator(stats.getLong(UnnestOperator.StatKey.EMITTED_ROWS));
+ }
};
+ private static final Map<Integer, Type> ID_TO_TYPE;
+
+ static {
+ Map<Integer, Type> map = new HashMap<>();
+ for (Type type : values()) {
+ int id = type._id;
+ if (id < 0 || id > Byte.MAX_VALUE) {
+ throw new IllegalStateException("Operator type id must fit in a signed byte (0-127), but " + type
+ + " has id " + id);
+ }
+ Type existing = map.put(id, type);
+ if (existing != null) {
+ throw new IllegalStateException("Duplicate id " + id + " for types " + existing + " and " + type);
+ }
+ }
+ ID_TO_TYPE = Map.copyOf(map);
+ }
+
+ private final int _id;
private final Class _statKeyClass;
Review Comment:
The field `_statKeyClass` is missing a generic type parameter. It should be declared as `Class<? extends StatMap.Key>` to maintain type safety and be consistent with the constructor parameter 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] Fix multi-stage query stats serde compatibility issue caused by UNNEST [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #17590:
URL: https://github.com/apache/pinot/pull/17590
--
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