Open
Conversation
yashmayya
added
enhancement
yashmayya
force-pushed
the
polymorphic-window-functions
branch
from
e81fbd0 to
de1f0c3
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds type-specific window value aggregators (and corresponding factory dispatch) so window aggregations preserve precision for integral and BIG_DECIMAL types, and reduces boxing for MIN/MAX on primitive types in pinot-query-runtime.
Changes:
- Update
WindowValueAggregatorFactoryto dispatchSUM/MIN/MAXaggregators based oncolumnDataType.getStoredType(). - Add new polymorphic implementations:
SumLong*,SumBigDecimal*, primitiveMin/Max(Int|Long)*, andMin/MaxComparable*fallbacks; rename existing double-based implementations for clarity. - Add a comprehensive
WindowValueAggregatorTestsuite covering factory dispatch, null/removal behavior, and precision.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/WindowValueAggregatorFactory.java | Dispatch to type-specific SUM/MIN/MAX implementations using stored type. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/SumLongWindowValueAggregator.java | New long-accumulating SUM to avoid double precision loss for large integral values. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/SumDoubleWindowValueAggregator.java | Rename/refocus existing SUM implementation as double-based. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/SumBigDecimalWindowValueAggregator.java | New BigDecimal SUM implementation intended to preserve decimal precision. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/MinIntWindowValueAggregator.java | New primitive INT MIN with fastutil deque for sliding windows. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/MaxIntWindowValueAggregator.java | New primitive INT MAX with fastutil deque for sliding windows. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/MinLongWindowValueAggregator.java | New primitive LONG MIN with fastutil deque (precision-safe vs double). |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/MaxLongWindowValueAggregator.java | New primitive LONG MAX with fastutil deque (precision-safe vs double). |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/MinDoubleWindowValueAggregator.java | Rename existing MIN implementation as double-based. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/MaxDoubleWindowValueAggregator.java | Rename existing MAX implementation as double-based. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/MinComparableWindowValueAggregator.java | New Comparable-based MIN fallback to preserve non-double types. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/MaxComparableWindowValueAggregator.java | New Comparable-based MAX fallback to preserve non-double types. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/BoolAndWindowValueAggregator.java | Rename class to match file name and window-aggregator naming. |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/aggregate/BoolOrWindowValueAggregator.java | Rename class to match file name and window-aggregator naming. |
| pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/window/aggregate/WindowValueAggregatorTest.java | New unit tests covering factory dispatch and aggregator behaviors/precision. |
| if (value instanceof BigDecimal) { | ||
| return (BigDecimal) value; | ||
| } | ||
| return BigDecimal.valueOf(((Number) value).doubleValue()); |
Comment on lines
+470
to
+475
| @Test(expectedExceptions = UnsupportedOperationException.class) | ||
| public void testComparableMinRemovalUnsupported() { | ||
| WindowValueAggregator<Object> agg = new MinComparableWindowValueAggregator(false); | ||
| agg.addValue(1); | ||
| agg.removeValue(1); | ||
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18169 +/- ##
============================================
+ Coverage 63.03% 63.16% +0.13%
+ Complexity 1617 1616 -1
============================================
Files 3202 3221 +19
Lines 194717 195950 +1233
Branches 30046 30275 +229
============================================
+ Hits 122739 123775 +1036
- Misses 62250 62286 +36
- Partials 9728 9889 +161
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gortiz
reviewed
Apr 13, 2026
|
|
||
| @Override | ||
| public void addValue(@Nullable Object value) { | ||
| if (value != null) { |
Contributor
There was a problem hiding this comment.
Not introduced in this PR, but how do we handle null first/last?
gortiz
reviewed
Apr 13, 2026
Comment on lines
+53
to
+55
| while (!_deque.isEmpty() && _deque.lastLong() > longValue) { | ||
| _deque.dequeueLastLong(); | ||
| } |
Contributor
There was a problem hiding this comment.
Just for me to understand: It looks like in _deque we have N copies of the min value. It should be more efficient to just keep the min and the number of repetitions we had, right? I guess I'm missing something.
gortiz
approved these changes
Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WindowValueAggregatorFactoryby adding type-specific window value aggregator implementationsSumLongWindowValueAggregatorforINT/LONGto avoid precision loss when summing large long values (> 2^53)SumBigDecimalWindowValueAggregatorforBIG_DECIMALto preserve full decimal precisionMinIntWindowValueAggregator/MaxIntWindowValueAggregatorusing fastutilIntArrayFIFOQueueMinLongWindowValueAggregator/MaxLongWindowValueAggregatorusing fastutilLongArrayFIFOQueueMinComparableWindowValueAggregator/MaxComparableWindowValueAggregatoras fallback for types likeBIG_DECIMALcolumnDataType.getStoredType()instead of always using double-based aggregators