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 2025/10/23 00:12:53 UTC
[PR] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
yashmayya opened a new pull request, #17058:
URL: https://github.com/apache/pinot/pull/17058
- Like https://github.com/apache/pinot/pull/16980/, but for `LONG` columns.
- The primary goal here is to avoid the precision loss that comes with using double based computation for super large long values.
- Unlike the `STRING` case though, here we also need to take into account backward compatibility - when `MIN` / `MAX` / `SUM` are rewritten, they should still be able to use existing star-tree indexes that would have pre-aggregated double values.
- As a future enhancement, we'll also introduce type specific value aggregators so that the pre-aggregated star-tree index values can be the appropriate type instead of always being double. This enhancement will also need to be backward and forward compatible.
- Note that we'll no longer be able to rely on the stored type hack for aggregation function column when the above enhancement will be made and we'll also need to refactor the star-tree index matching logic a bit. For instance, `MINLONG` should be able to use a star-tree index built with pre-aggregated long values or double values.
--
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] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #17058:
URL: https://github.com/apache/pinot/pull/17058#issuecomment-3444516399
> Backward incompatible is fine. We need to find a way to make the tests pass
Yeah I've refactored the compat verifier test framework to support taking in multiple expected column types (older will be `DOUBLE`, newer will be `LONG`) and ignoring column name mismatch (we don't care about `min(longCol)` vs `minlong(longCol)` for instance, and we've made similar changes in the past like `LONG` -> `BIGINT` for casts). Otherwise these tests would keep failing when run versus older versions.
--
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] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #17058:
URL: https://github.com/apache/pinot/pull/17058#issuecomment-3444952249
I've pushed another commit introducing a new query option / broker config that gates this auto rewrite functionality for both SSE and MSE. It currently defaults to false, and we can either change the default to true in a future release or else remove the query option / broker config directly.
--
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] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17058:
URL: https://github.com/apache/pinot/pull/17058#discussion_r2460713144
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/statement/AggregateFunctionRewriteOptimizer.java:
##########
@@ -27,9 +27,17 @@
import org.apache.pinot.spi.data.FieldSpec;
import org.apache.pinot.spi.data.Schema;
-/**
- * Rewrites aggregate functions to type-specific versions in order to support polymorphic functions.
- */
+///
Review Comment:
(nit) Remove redundant leading/trailing empty line javadoc
##########
pinot-controller/src/main/java/org/apache/pinot/controller/recommender/rules/impl/AggregateMetricsRule.java:
##########
@@ -67,8 +67,10 @@ private boolean shouldAggregate(InputManager inputManager) {
continue;
}
FunctionContext funcCtx = selectExpr.getFunction();
- if (selectExpr.getType() != ExpressionContext.Type.FUNCTION || !funcCtx.getFunctionName()
- .equalsIgnoreCase("SUM") || hasNonMetricArguments(funcCtx.getArguments(), metricNames)) {
+ if (selectExpr.getType() != ExpressionContext.Type.FUNCTION || (
+ !funcCtx.getFunctionName().equalsIgnoreCase("SUM") && !funcCtx.getFunctionName().equalsIgnoreCase("SUMINT")
Review Comment:
@sajjad-moradi Can you help take a look at this part? Should we also include `SUM_PRECISION` 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] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17058:
URL: https://github.com/apache/pinot/pull/17058#issuecomment-3435011777
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17058?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 `60.34483%` with `23 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.29%. Comparing base ([`c405193`](https://app.codecov.io/gh/apache/pinot/commit/c405193360a56a31266296846938afbc91d5f4b9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`acad037`](https://app.codecov.io/gh/apache/pinot/commit/acad037f883a726eda93c45167aaf353b132545d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17058?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...r/statement/AggregateFunctionRewriteOptimizer.java](https://app.codecov.io/gh/apache/pinot/pull/17058?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Foptimizer%2Fstatement%2FAggregateFunctionRewriteOptimizer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvc3RhdGVtZW50L0FnZ3JlZ2F0ZUZ1bmN0aW9uUmV3cml0ZU9wdGltaXplci5qYXZh) | 37.50% | [12 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/rel/rules/PinotAggregateFunctionRewriteRule.java](https://app.codecov.io/gh/apache/pinot/pull/17058?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcalcite%2Frel%2Frules%2FPinotAggregateFunctionRewriteRule.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2FsY2l0ZS9yZWwvcnVsZXMvUGlub3RBZ2dyZWdhdGVGdW5jdGlvblJld3JpdGVSdWxlLmphdmE=) | 83.87% | [4 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../index/startree/AggregationFunctionColumnPair.java](https://app.codecov.io/gh/apache/pinot/pull/17058?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Findex%2Fstartree%2FAggregationFunctionColumnPair.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3N0YXJ0cmVlL0FnZ3JlZ2F0aW9uRnVuY3Rpb25Db2x1bW5QYWlyLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17058?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 (c405193) and HEAD (acad037). Click for more details.
>
> <details><summary>HEAD has 28 uploads less than BASE</summary>
>
>| Flag | BASE (c405193) | HEAD (acad037) |
>|------|------|------|
>|java-21|4|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|8|1|
>|java-11|4|0|
>|unittests2|2|0|
>|integration|4|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17058 +/- ##
============================================
- Coverage 63.48% 56.29% -7.20%
+ Complexity 1425 701 -724
============================================
Files 3085 2416 -669
Lines 182274 135070 -47204
Branches 27968 21569 -6399
============================================
- Hits 115722 76037 -39685
+ Misses 57626 52676 -4950
+ Partials 8926 6357 -2569
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17058/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/17058/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/17058/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/17058/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/17058/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/17058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.29% <60.34%> (-7.16%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.29% <60.34%> (-7.20%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.29% <60.34%> (-7.20%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.29% <60.34%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17058/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/17058?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] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #17058:
URL: https://github.com/apache/pinot/pull/17058#issuecomment-3469202280
Unrelated test failures, merging this.
--
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] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #17058:
URL: https://github.com/apache/pinot/pull/17058
--
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] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #17058:
URL: https://github.com/apache/pinot/pull/17058#issuecomment-3439285481
@Jackie-Jiang thoughts on the backward compatibility issue here? Are we okay with changing the return type for `MIN(longCol)` / `MAX(longCol)` from `DOUBLE` to `LONG` in SSE? It's already `LONG` for MSE (the aggregation result is currently cast form double to long).
Also what about `SUM(longCol)`? Right now, we return `DOUBLE` in SSE which might lead to precision loss, but changing it to `LONG` here will mean a tradeoff for potential overflow. Again, in MSE today we cast the returned double value to long.
--
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] Automatically rewrite MIN / MAX / SUM on long columns to MINLONG / MAXLONG / SUMLONG [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #17058:
URL: https://github.com/apache/pinot/pull/17058#issuecomment-3444571886
Actually, thinking about this some more, I don't think it's safe to do this rewrite automatically without a two phase release (https://github.com/apache/pinot/pull/17001 hasn't been released yet). If brokers are upgraded before servers (which is our recommended upgrade order - https://docs.pinot.apache.org/operators/operating-pinot/upgrading-pinot-cluster), then the query can be rewritten by the broker to use aggregation functions that are not yet supported by the servers which would lead to query failures during the upgrade process. IMO we need to first enable this rewrite using a query option or broker config which defaults to false. After the next release, we can change the default to true, or remove the option completely and do the rewrite automatically by default.
--
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