You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "KKcorps (via GitHub)" <gi...@apache.org> on 2023/02/17 07:12:33 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #10299: Remove duplicate stats aggregator from V2 query metrics
KKcorps opened a new pull request, #10299:
URL: https://github.com/apache/pinot/pull/10299
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins 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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1110351153
##########
pinot-core/src/test/java/org/apache/pinot/core/query/reduce/StreamingReduceServiceTest.java:
##########
@@ -56,7 +56,7 @@ public void testThreadExceptionTransfer() {
threadPoolService,
ImmutableMap.of(routingInstance, mockedResponse),
1000,
- mock(BaseReduceService.ExecutionStatsAggregator.class));
+ mock(ExecutionStatsAggregator.class));
Review Comment:
could we add some test for both v1 and v2 engine for collecting and aggregating stats via different metadata blocks?
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10299:
URL: https://github.com/apache/pinot/pull/10299#issuecomment-1434255863
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10299](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f495bb0) into [master](https://codecov.io/gh/apache/pinot/commit/a0162559b5dfbf5ac56c342afd5d576532b3dc3d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a016255) will **decrease** coverage by `56.66%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10299 +/- ##
=============================================
- Coverage 70.36% 13.71% -56.66%
+ Complexity 5827 182 -5645
=============================================
Files 2016 1962 -54
Lines 109635 107033 -2602
Branches 16680 16373 -307
=============================================
- Hits 77145 14677 -62468
- Misses 27073 91191 +64118
+ Partials 5417 1165 -4252
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.71% <0.00%> (+0.01%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...requesthandler/MultiStageBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (-74.36%)` | :arrow_down: |
| [...che/pinot/core/query/reduce/BaseReduceService.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQmFzZVJlZHVjZVNlcnZpY2UuamF2YQ==) | `0.00% <ø> (-97.40%)` | :arrow_down: |
| [...ot/core/query/reduce/ExecutionStatsAggregator.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvRXhlY3V0aW9uU3RhdHNBZ2dyZWdhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...query/runtime/operator/MailboxReceiveOperator.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94UmVjZWl2ZU9wZXJhdG9yLmphdmE=) | `0.00% <ø> (-95.46%)` | :arrow_down: |
| [...he/pinot/query/runtime/operator/OperatorStats.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9PcGVyYXRvclN0YXRzLmphdmE=) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [...rg/apache/pinot/query/service/QueryDispatcher.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9RdWVyeURpc3BhdGNoZXIuamF2YQ==) | `0.00% <0.00%> (-69.75%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/common/CustomObject.java](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vQ3VzdG9tT2JqZWN0LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1589 more](https://codecov.io/gh/apache/pinot/pull/10299?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113379771
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/AggregateOperator.java:
##########
@@ -83,14 +84,22 @@ public class AggregateOperator extends MultiStageOperator {
public AggregateOperator(MultiStageOperator inputOperator, DataSchema dataSchema, List<RexExpression> aggCalls,
List<RexExpression> groupSet, DataSchema inputSchema, long requestId, int stageId) {
this(inputOperator, dataSchema, aggCalls, groupSet, inputSchema, AggregateOperator.Accumulator.MERGERS, requestId,
- stageId);
+ stageId, null);
+ }
+
+ public AggregateOperator(MultiStageOperator inputOperator, DataSchema dataSchema, List<RexExpression> aggCalls,
Review Comment:
Mostly test classes
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1110802829
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxReceiveOperator.java:
##########
@@ -164,7 +164,6 @@ protected TransferableBlock getNextBlock() {
return block;
} else {
if (!block.getResultMetadata().isEmpty()) {
- _operatorStats.clearExecutionStats();
Review Comment:
Previously we were aggregating stats, hence there was need to clear them. Now, we are just replacing the stats.
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1110597057
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorStats.java:
##########
@@ -61,11 +61,11 @@ public void recordRow(int numBlock, int numRows) {
}
public void recordExecutionStats(Map<String, String> executionStats) {
- _statsAggregator.aggregate(executionStats);
+ _executionStats = executionStats;
Review Comment:
Makes sense. I was thinking about that as well.
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1110352966
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorStats.java:
##########
@@ -61,11 +61,11 @@ public void recordRow(int numBlock, int numRows) {
}
public void recordExecutionStats(Map<String, String> executionStats) {
- _statsAggregator.aggregate(executionStats);
+ _executionStats = executionStats;
Review Comment:
could we think of a way to incorporate `_numBlock` and `_numRows` in the executionStats?
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1112099690
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorStats.java:
##########
@@ -61,11 +63,14 @@ public void recordRow(int numBlock, int numRows) {
}
public void recordExecutionStats(Map<String, String> executionStats) {
- _statsAggregator.aggregate(executionStats);
+ _executionStats = executionStats;
}
public Map<String, String> getExecutionStats() {
- return _statsAggregator.getStats();
+ _executionStats.put(OperatorUtils.NUM_BLOCKS, String.valueOf(_numBlock));
+ _executionStats.put(OperatorUtils.NUM_ROWS, String.valueOf(_numRows));
+ _executionStats.put(OperatorUtils.WALL_TIME, String.valueOf(_executeStopwatch.elapsed(TimeUnit.MILLISECONDS)));
Review Comment:
this is technically thread-time not WALL_TIME. let's change the naming
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113801542
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -260,6 +294,48 @@ private static Object[][] testResourceQueryTestCaseProviderBoth()
return providerContent.toArray(new Object[][]{});
}
+ @DataProvider
+ private static Object[][] testResourceQueryTestCaseProviderWithMetadata()
+ throws Exception {
+ Map<String, QueryTestCase> testCaseMap = getTestCases();
+ List<Object[]> providerContent = new ArrayList<>();
+ for (Map.Entry<String, QueryTestCase> testCaseEntry : testCaseMap.entrySet()) {
+ String testCaseName = testCaseEntry.getKey();
+ if (testCaseEntry.getValue()._ignored) {
+ continue;
+ }
+
+ List<QueryTestCase.Query> queryCases = testCaseEntry.getValue()._queries;
+ for (QueryTestCase.Query queryCase : queryCases) {
+ if (queryCase._ignored) {
+ continue;
+ }
+
+ if (queryCase._outputs != null) {
+ String sql = replaceTableName(testCaseName, queryCase._sql);
+ if (!sql.contains("basic_test")) {
+ continue;
+ }
Review Comment:
technically you can test this stats with numOfSegments on all test cases. also you can directly add a test to QueryRunnerTest instead of doing it here. but I will leave this up to you..
my thinking is: since you are not testing many different scenarios. it is best to just have 1 `@Test` section in QueryRunnerTest
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113259046
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/AggregateOperator.java:
##########
@@ -83,14 +84,22 @@ public class AggregateOperator extends MultiStageOperator {
public AggregateOperator(MultiStageOperator inputOperator, DataSchema dataSchema, List<RexExpression> aggCalls,
List<RexExpression> groupSet, DataSchema inputSchema, long requestId, int stageId) {
this(inputOperator, dataSchema, aggCalls, groupSet, inputSchema, AggregateOperator.Accumulator.MERGERS, requestId,
- stageId);
+ stageId, null);
+ }
+
+ public AggregateOperator(MultiStageOperator inputOperator, DataSchema dataSchema, List<RexExpression> aggCalls,
Review Comment:
can we replace the constructor by adding virtual server instead of creating one? which function is still using the constructor without the virtual server address?
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113260236
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java:
##########
@@ -82,7 +82,7 @@ public MailboxSendOperator(MailboxService<TransferableBlock> mailboxService,
RelDistribution.Type exchangeType, KeySelector<Object[], Object[]> keySelector,
MailboxIdGenerator mailboxIdGenerator, BlockExchangeFactory blockExchangeFactory, long jobId, int senderStageId,
int receiverStageId) {
- super(jobId, senderStageId);
+ super(jobId, senderStageId, null);
Review Comment:
why are we setting it to null?
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -111,6 +112,37 @@ protected List<Object[]> queryRunner(String sql) {
queryPlan.getQueryResultFields(), queryPlan.getQueryStageMap().get(0).getDataSchema()).getRows();
}
+ protected List<Object[]> queryRunner(String sql, ExecutionStatsAggregator executionStatsAggregator) {
Review Comment:
simply make the normal queryRunner API call this one with a null executionStatsAggregator?
the only difference is the last step when reduceMailboxReceive yes?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -70,7 +74,12 @@ public TransferableBlock nextBlock() {
}
if (!_operatorStats.getExecutionStats().isEmpty()) {
- String operatorId = Joiner.on("_").join(toExplainString(), _requestId, _stageId);
+ String operatorId;
+ if (_serverAddress != null) {
+ operatorId = Joiner.on("_").join(toExplainString(), _requestId, _stageId, _serverAddress);
+ } else {
+ operatorId = Joiner.on("_").join(toExplainString(), _requestId, _stageId);
+ }
Review Comment:
these can be moved to constructor right?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -70,7 +74,12 @@ public TransferableBlock nextBlock() {
}
if (!_operatorStats.getExecutionStats().isEmpty()) {
Review Comment:
the TODO above can be removed ^
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxReceiveOperator.java:
##########
@@ -164,8 +164,16 @@ protected TransferableBlock getNextBlock() {
return block;
} else {
if (!block.getResultMetadata().isEmpty()) {
- _operatorStats.clearExecutionStats();
_operatorStatsMap.putAll(block.getResultMetadata());
+// for(String operatorId: block.getResultMetadata().keySet()) {
+// //TODO: Remove this hack!
+// if (!operatorId.contains("serverId")) {
+// _operatorStatsMap.put(operatorId + "_serverId" + mailboxId.getFromHost(),
+// block.getResultMetadata().get(operatorId));
+// } else {
+// _operatorStatsMap.put(operatorId, block.getResultMetadata().get(operatorId));
+// }
+// }
Review Comment:
remove
##########
pinot-query-runtime/src/test/resources/metadata_queries/BasicQuery.json:
##########
@@ -0,0 +1,68 @@
+{
Review Comment:
why are we duplicating the ResourceBasedTest?
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1110352966
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorStats.java:
##########
@@ -61,11 +61,11 @@ public void recordRow(int numBlock, int numRows) {
}
public void recordExecutionStats(Map<String, String> executionStats) {
- _statsAggregator.aggregate(executionStats);
+ _executionStats = executionStats;
Review Comment:
could we think of a way to incorporate `_numBlock` and `_numRows` in the executionStats?
currently the executionStats is a Map which is generic enough, but the stats aggregator is still recognizing hard-coded keys for stats. should we add some that's only valid in V2 and some only valid in V1?
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113810048
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -260,6 +294,48 @@ private static Object[][] testResourceQueryTestCaseProviderBoth()
return providerContent.toArray(new Object[][]{});
}
+ @DataProvider
+ private static Object[][] testResourceQueryTestCaseProviderWithMetadata()
+ throws Exception {
+ Map<String, QueryTestCase> testCaseMap = getTestCases();
+ List<Object[]> providerContent = new ArrayList<>();
+ for (Map.Entry<String, QueryTestCase> testCaseEntry : testCaseMap.entrySet()) {
+ String testCaseName = testCaseEntry.getKey();
+ if (testCaseEntry.getValue()._ignored) {
+ continue;
+ }
+
+ List<QueryTestCase.Query> queryCases = testCaseEntry.getValue()._queries;
+ for (QueryTestCase.Query queryCase : queryCases) {
+ if (queryCase._ignored) {
+ continue;
+ }
+
+ if (queryCase._outputs != null) {
+ String sql = replaceTableName(testCaseName, queryCase._sql);
+ if (!sql.contains("basic_test")) {
+ continue;
+ }
Review Comment:
Can't do for all test cases since the metrics contain only segments queries from the servers but doesn't account for the segments that are pruned by the brokers. So in some cases the test fails.
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113476153
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/AggregateOperator.java:
##########
@@ -83,14 +84,22 @@ public class AggregateOperator extends MultiStageOperator {
public AggregateOperator(MultiStageOperator inputOperator, DataSchema dataSchema, List<RexExpression> aggCalls,
List<RexExpression> groupSet, DataSchema inputSchema, long requestId, int stageId) {
this(inputOperator, dataSchema, aggCalls, groupSet, inputSchema, AggregateOperator.Accumulator.MERGERS, requestId,
- stageId);
+ stageId, null);
+ }
+
+ public AggregateOperator(MultiStageOperator inputOperator, DataSchema dataSchema, List<RexExpression> aggCalls,
Review Comment:
Done.
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113484775
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -111,6 +112,37 @@ protected List<Object[]> queryRunner(String sql) {
queryPlan.getQueryResultFields(), queryPlan.getQueryStageMap().get(0).getDataSchema()).getRows();
}
+ protected List<Object[]> queryRunner(String sql, ExecutionStatsAggregator executionStatsAggregator) {
Review Comment:
Done.
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113382185
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/AggregateOperator.java:
##########
@@ -83,14 +84,22 @@ public class AggregateOperator extends MultiStageOperator {
public AggregateOperator(MultiStageOperator inputOperator, DataSchema dataSchema, List<RexExpression> aggCalls,
List<RexExpression> groupSet, DataSchema inputSchema, long requestId, int stageId) {
this(inputOperator, dataSchema, aggCalls, groupSet, inputSchema, AggregateOperator.Accumulator.MERGERS, requestId,
- stageId);
+ stageId, null);
+ }
+
+ public AggregateOperator(MultiStageOperator inputOperator, DataSchema dataSchema, List<RexExpression> aggCalls,
Review Comment:
oh. i see. in this case lets make it clean -- there's no need to keep additional public constructor just for test purposes.
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1110350053
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxReceiveOperator.java:
##########
@@ -164,7 +164,6 @@ protected TransferableBlock getNextBlock() {
return block;
} else {
if (!block.getResultMetadata().isEmpty()) {
- _operatorStats.clearExecutionStats();
Review Comment:
why is this necessary previously? i thought the stats aggregator is only created once right?
--
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
[GitHub] [pinot] KKcorps merged pull request #10299: Remove duplicate stats aggregator from V2 query metrics
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps merged PR #10299:
URL: https://github.com/apache/pinot/pull/10299
--
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