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 2023/10/09 07:28:45 UTC
[PR] Enhance Broker reducer to handle expression format change [pinot]
Jackie-Jiang opened a new pull request, #11762:
URL: https://github.com/apache/pinot/pull/11762
Enhance broker reducer to not rely on the column names returned from the servers. This is required to support future expression format change (e.g. do not include single quotes around numeric literals).
- Drop incompatible data tables for all query types to prevent type incompatible exceptions
- Remove special handling of distinct data table format introduced before releasing `0.12`
--
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 Broker reducer to handle expression format change [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11762:
URL: https://github.com/apache/pinot/pull/11762#discussion_r1355936238
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java:
##########
@@ -99,8 +119,23 @@ public BrokerResponseNative reduceOnDataTable(BrokerRequest brokerRequest, Broke
// Set execution statistics and Update broker metrics.
aggregator.setStats(rawTableName, brokerResponseNative, brokerMetrics);
+ // Report the servers with conflicting data schema.
+ if (!serversWithConflictingDataSchema.isEmpty()) {
+ String errorMessage =
+ String.format("%s: responses for table: %s from servers: %s got dropped due to data schema inconsistency.",
+ QueryException.MERGE_RESPONSE_ERROR.getMessage(), tableName, serversWithConflictingDataSchema);
+ LOGGER.warn(errorMessage);
+ if (brokerMetrics != null) {
+ brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.RESPONSE_MERGE_EXCEPTIONS, 1L);
+ }
+ brokerResponseNative.addToExceptions(
+ new QueryProcessingException(QueryException.MERGE_RESPONSE_ERROR_CODE, errorMessage));
+ }
+
// NOTE: When there is no cached data schema, that means all servers encountered exception. In such case, return the
// response with metadata only.
+ DataSchema cachedDataSchema =
Review Comment:
The comment is about cached data schema, so I feel it is fine putting it here
--
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 Broker reducer to handle expression format change [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11762:
URL: https://github.com/apache/pinot/pull/11762#discussion_r1355935410
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/AggregationDataTableReducer.java:
##########
@@ -47,9 +45,12 @@
@SuppressWarnings({"rawtypes", "unchecked"})
public class AggregationDataTableReducer implements DataTableReducer {
private final QueryContext _queryContext;
+ private final AggregationFunction[] _aggregationFunctions;
- AggregationDataTableReducer(QueryContext queryContext) {
+ public AggregationDataTableReducer(QueryContext queryContext) {
Review Comment:
Technically it is not required to be public, but it doesn't hurt since we don't really have a reason to prevent it from being reused
--
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 Broker reducer to handle expression format change [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11762:
URL: https://github.com/apache/pinot/pull/11762
--
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 Broker reducer to handle expression format change [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11762:
URL: https://github.com/apache/pinot/pull/11762#discussion_r1355312354
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/AggregationDataTableReducer.java:
##########
@@ -47,9 +45,12 @@
@SuppressWarnings({"rawtypes", "unchecked"})
public class AggregationDataTableReducer implements DataTableReducer {
private final QueryContext _queryContext;
+ private final AggregationFunction[] _aggregationFunctions;
- AggregationDataTableReducer(QueryContext queryContext) {
+ public AggregationDataTableReducer(QueryContext queryContext) {
Review Comment:
this and below: is public access needed? (not seeing any changes in how contructors are used
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java:
##########
@@ -99,8 +119,23 @@ public BrokerResponseNative reduceOnDataTable(BrokerRequest brokerRequest, Broke
// Set execution statistics and Update broker metrics.
aggregator.setStats(rawTableName, brokerResponseNative, brokerMetrics);
+ // Report the servers with conflicting data schema.
+ if (!serversWithConflictingDataSchema.isEmpty()) {
+ String errorMessage =
+ String.format("%s: responses for table: %s from servers: %s got dropped due to data schema inconsistency.",
+ QueryException.MERGE_RESPONSE_ERROR.getMessage(), tableName, serversWithConflictingDataSchema);
+ LOGGER.warn(errorMessage);
+ if (brokerMetrics != null) {
+ brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.RESPONSE_MERGE_EXCEPTIONS, 1L);
+ }
+ brokerResponseNative.addToExceptions(
+ new QueryProcessingException(QueryException.MERGE_RESPONSE_ERROR_CODE, errorMessage));
+ }
+
// NOTE: When there is no cached data schema, that means all servers encountered exception. In such case, return the
// response with metadata only.
+ DataSchema cachedDataSchema =
Review Comment:
(nit) notes are for line 139?
--
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 Broker reducer to handle expression format change [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11762:
URL: https://github.com/apache/pinot/pull/11762#issuecomment-1752543552
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11762](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2214fb8) into [master](https://app.codecov.io/gh/apache/pinot/commit/52d16f76685929eb46d001f0a435beca204da6cb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (52d16f7) will **decrease** coverage by `48.72%`.
> Report is 1 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11762 +/- ##
=============================================
- Coverage 63.12% 14.41% -48.72%
+ Complexity 1118 201 -917
=============================================
Files 2342 2343 +1
Lines 125917 125976 +59
Branches 19370 19388 +18
=============================================
- Hits 79487 18158 -61329
- Misses 40770 106285 +65515
+ Partials 5660 1533 -4127
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11762/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/11762/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%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11762/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/11762/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/11762/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.40% <0.00%> (-48.69%)` | :arrow_down: |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11762/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%> (-14.42%)` | :arrow_down: |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11762/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.41% <0.00%> (-48.55%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11762/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.41% <0.00%> (-48.72%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11762/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.41% <0.00%> (-48.71%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11762/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/11762/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.41% <0.00%> (-0.03%)` | :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](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...not/core/query/reduce/GroupByDataTableReducer.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvR3JvdXBCeURhdGFUYWJsZVJlZHVjZXIuamF2YQ==) | `0.00% <0.00%> (-67.24%)` | :arrow_down: |
| [...re/query/reduce/SelectionOnlyStreamingReducer.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvU2VsZWN0aW9uT25seVN0cmVhbWluZ1JlZHVjZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...core/query/selection/SelectionOperatorService.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zZWxlY3Rpb24vU2VsZWN0aW9uT3BlcmF0b3JTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (-93.75%)` | :arrow_down: |
| [...t/core/query/reduce/SelectionDataTableReducer.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvU2VsZWN0aW9uRGF0YVRhYmxlUmVkdWNlci5qYXZh) | `0.00% <0.00%> (-67.50%)` | :arrow_down: |
| [...e/operator/LeafStageTransferableBlockOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9MZWFmU3RhZ2VUcmFuc2ZlcmFibGVCbG9ja09wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-87.66%)` | :arrow_down: |
| [...e/pinot/core/query/reduce/BrokerReduceService.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQnJva2VyUmVkdWNlU2VydmljZS5qYXZh) | `0.00% <0.00%> (-84.00%)` | :arrow_down: |
| [...core/query/reduce/AggregationDataTableReducer.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQWdncmVnYXRpb25EYXRhVGFibGVSZWR1Y2VyLmphdmE=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [...ot/core/query/reduce/DistinctDataTableReducer.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvRGlzdGluY3REYXRhVGFibGVSZWR1Y2VyLmphdmE=) | `0.00% <0.00%> (-87.70%)` | :arrow_down: |
| [...inot/core/query/reduce/ReducerDataSchemaUtils.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUmVkdWNlckRhdGFTY2hlbWFVdGlscy5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...t/core/query/selection/SelectionOperatorUtils.java](https://app.codecov.io/gh/apache/pinot/pull/11762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zZWxlY3Rpb24vU2VsZWN0aW9uT3BlcmF0b3JVdGlscy5qYXZh) | `0.00% <0.00%> (-89.17%)` | :arrow_down: |
... and [1507 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11762/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