You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shauryachats (via GitHub)" <gi...@apache.org> on 2026/02/11 01:12:53 UTC
[PR] [timeseries] Adding timeUsedMs to query statistics [pinot]
shauryachats opened a new pull request, #17676:
URL: https://github.com/apache/pinot/pull/17676
(no comment)
--
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] [timeseries] Adding timeUsedMs to query statistics [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #17676:
URL: https://github.com/apache/pinot/pull/17676#discussion_r2810605378
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/TimeSeriesRequestHandler.java:
##########
@@ -166,6 +167,9 @@ public TimeSeriesBlock handleTimeSeriesRequest(String lang, String rawQueryParam
timeSeriesBlock = _queryDispatcher.submitAndGet(requestContext.getRequestId(), dispatchablePlan,
timeSeriesRequest.getTimeout().toMillis(), requestContext);
+
+ long endToEndTimeMs = System.currentTimeMillis() - queryStartTime;
Review Comment:
Moved it to the `finally` block. The error cases would still have zero time used since we only throw exceptions.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/timeseries/TimeSeriesExchangeReceiveOperator.java:
##########
@@ -210,6 +218,21 @@ private void mergeStats(Map<String, String> aggregatedStats, Map<String, String>
}
}
}
+
+ for (DataTable.MetadataKey statKey : MIN_STATS_KEYS) {
+ String key = statKey.getName();
+ String existingValue = aggregatedStats.getOrDefault(key, "0");
Review Comment:
Good catch! Updated to `Long.MAX_VALUE`.
--
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] [timeseries] Adding timeUsedMs to query statistics [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17676:
URL: https://github.com/apache/pinot/pull/17676#issuecomment-3881699259
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17676?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 `50.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.58%. Comparing base ([`b03be81`](https://app.codecov.io/gh/apache/pinot/commit/b03be818f9f2bf5493b761be4c817460a58b6c88?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`34df89a`](https://app.codecov.io/gh/apache/pinot/commit/34df89a7de98648e215f049ebfac6860ba77109b?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/17676?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...mmon/response/mapper/TimeSeriesResponseMapper.java](https://app.codecov.io/gh/apache/pinot/pull/17676?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fresponse%2Fmapper%2FTimeSeriesResponseMapper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvbWFwcGVyL1RpbWVTZXJpZXNSZXNwb25zZU1hcHBlci5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17676?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 (b03be81) and HEAD (34df89a). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (b03be81) | HEAD (34df89a) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|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 #17676 +/- ##
============================================
- Coverage 63.25% 55.58% -7.67%
+ Complexity 1499 720 -779
============================================
Files 3174 2475 -699
Lines 190373 140302 -50071
Branches 29089 22349 -6740
============================================
- Hits 120415 77992 -42423
+ Misses 60624 55725 -4899
+ Partials 9334 6585 -2749
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17676/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/17676/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/17676/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/17676/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/17676/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/17676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.58% <50.00%> (-7.64%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.58% <50.00%> (-7.67%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.58% <50.00%> (-7.67%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.58% <50.00%> (-0.07%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17676/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/17676?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] [timeseries] Adding timeUsedMs to query statistics [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #17676:
URL: https://github.com/apache/pinot/pull/17676#discussion_r2807042998
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/TimeSeriesRequestHandler.java:
##########
@@ -166,6 +167,9 @@ public TimeSeriesBlock handleTimeSeriesRequest(String lang, String rawQueryParam
timeSeriesBlock = _queryDispatcher.submitAndGet(requestContext.getRequestId(), dispatchablePlan,
timeSeriesRequest.getTimeout().toMillis(), requestContext);
+
+ long endToEndTimeMs = System.currentTimeMillis() - queryStartTime;
Review Comment:
We should return time for every case. There are some if conditions like on L161 which will miss this.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/timeseries/TimeSeriesExchangeReceiveOperator.java:
##########
@@ -210,6 +218,21 @@ private void mergeStats(Map<String, String> aggregatedStats, Map<String, String>
}
}
}
+
+ for (DataTable.MetadataKey statKey : MIN_STATS_KEYS) {
+ String key = statKey.getName();
+ String existingValue = aggregatedStats.getOrDefault(key, "0");
Review Comment:
Doesn't this mean that the result will always be 0?
--
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] [timeseries] Adding timeUsedMs to query statistics [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana merged PR #17676:
URL: https://github.com/apache/pinot/pull/17676
--
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