Skip to content

Add schema-bound polymorphic aggregation across query engines - #19523

Open
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/codex/sse-aggregate-type-binding
Open

Add schema-bound polymorphic aggregation across query engines#19523
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/codex/sse-aggregate-type-binding

Conversation

@xiangfu0

Copy link
Copy Markdown
Contributor

Polymorphic aggregates currently rely on fixed result types, explicit SQL type arguments, or types discovered from data blocks. This makes empty responses unreliable and can lose logical BOOLEAN/TIMESTAMP types between planning, execution, and reduction.

This change resolves argument and result types once from the input schema and carries an immutable binding through SSE requests and MSE plans. Aggregation providers are discovered through ServiceLoader, so another declared polymorphic aggregate can supply its type rule and implementation without adding factory, planner-stage, or reducer switch branches.

  • Support inferred two-argument FIRST_WITH_TIME/LAST_WITH_TIME and one-argument ARRAY_AGG, with an optional boolean distinct argument.
  • Preserve logical types for nonnumeric MODE, ANY_VALUE, and the existing SSE EXPR_MIN/EXPR_MAX rewrite.
  • Keep bindings through expression overrides, gapfill, grouping, post-aggregation, broker-pruned responses, and LIMIT 0. Read older physical segment types through the bound type's conversion getters.
  • Preserve null projections and all tied rows in serialized ExprMin/Max merges; add the extension guide in pinot-core/README.md.

Existing explicit-type FIRST/LAST and ARRAY_AGG forms remain supported. Numeric MODE keeps its DOUBLE result and AVG tie reducer; fixed numeric aggregate contracts are unchanged. Existing calls retain legacy execution when a native transform has no schema-only type rule; the new inferred overloads require resolvable input metadata. Binding fields are optional, but the new inferred overloads and logical-type semantics require upgraded brokers and execution workers. Public ExprMin/Max execution remains SSE-only through its existing configured rewrite.

Validation: 577 focused tests across 39 classes passed in the final reactor run, including both planners, all three MSE fixture execution modes, raw/dictionary segments, distributed merges, null/empty results, and broker-only responses. Spotless, Checkstyle, and license checks passed on all six affected modules. Compilation with warnings enabled used JDK 25 and a local classpath workaround for the missing JetBrains annotation dependency in unchanged compression code; no project dependency was changed. CI and maintainer review are pending.

@xiangfu0 xiangfu0 added extension-point Adds or modifies an extension/SPI point feature New functionality labels Sep 10, 2026
@codecov-commenter

codecov-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.78725% with 205 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.88%. Comparing base (6aaa040) to head (cddb856).
⚠️ Report is 22 commits behind head on master.

Files with missing lines Patch % Lines
...core/query/aggregation/ExpressionTypeResolver.java 69.59% 19 Missing and 26 partials ⚠️
...che/pinot/segment/spi/AggregationFunctionType.java 63.01% 5 Missing and 22 partials ⚠️
...mmon/function/AggregationFunctionTypeResolver.java 76.27% 8 Missing and 6 partials ⚠️
.../aggregation/GapfillAggregationFunctionBinder.java 87.34% 3 Missing and 7 partials ⚠️
...sthandler/BaseSingleStageBrokerRequestHandler.java 57.14% 7 Missing and 2 partials ⚠️
.../aggregation/function/ModeAggregationFunction.java 92.80% 2 Missing and 7 partials ⚠️
...ation/function/array/ArrayAggFunctionProvider.java 82.97% 2 Missing and 6 partials ⚠️
...apache/pinot/calcite/rel/rules/PinotRuleUtils.java 68.18% 4 Missing and 3 partials ⚠️
...inot/query/planner/logical/RexExpressionUtils.java 33.33% 6 Missing ⚠️
...t/common/request/context/AggregateCallBinding.java 72.22% 2 Missing and 3 partials ⚠️
... and 28 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19523      +/-   ##
============================================
+ Coverage     67.72%   67.88%   +0.15%     
  Complexity     1430     1430              
============================================
  Files          3489     3499      +10     
  Lines        224634   225772    +1138     
  Branches      35468    35747     +279     
============================================
+ Hits         152130   153256    +1126     
+ Misses        60485    60408      -77     
- Partials      12019    12108      +89     
Flag Coverage Δ
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 67.88% <80.78%> (+0.15%) ⬆️
lane-a 100.00% <ø> (ø)
lane-b 0.00% <ø> (ø)
temurin 67.88% <80.78%> (+0.15%) ⬆️
unittests 67.87% <80.78%> (+0.15%) ⬆️
unittests1 58.14% <80.91%> (+0.36%) ⬆️
unittests2 39.54% <26.42%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extension-point Adds or modifies an extension/SPI point feature New functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants