feat(sql): add ntile, cume_dist, and nth_value window functions - #6925
Merged
Conversation
jovfer
added
New feature
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Distribute rows into n approximately equal buckets. First totalRows % n buckets get ceil(totalRows/n) rows, remaining get floor(totalRows/n). Two-pass: pass1 counts rows per partition, pass2 computes bucket assignments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cumulative distribution: fraction of partition rows at or before the current row. Peers (rows with the same ORDER BY value) get the same result. Two-pass: pass1 stores rank, pass2 flushes peer groups with countAtOrBefore / totalRows. No ORDER BY returns 1.0 for all rows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Return the value at position n (1-based) within the window frame. Supports all frame modes: ROWS, RANGE, PARTITION BY, unbounded and bounded frames. Returns NULL when n exceeds the current frame size. IGNORE NULLS is not yet supported. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add toPlan() to 4 nth_value inner classes that were missing n param - Add test for nth_value IGNORE NULLS rejection - Add tests for nth_value RANGE frame (partitioned and non-partitioned) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The original commits added the factory classes but forgot to register them in function_list.txt, so cume_dist() and nth_value() were not resolvable from SQL. Add both entries next to ntile(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jovfer
force-pushed
the
sm_ntile_cumedist_nthvalue
branch
from
April 15, 2026 13:58
c58d750 to
b989be7
Compare
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
1 similar comment
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
- Remove unreachable IGNORE NULLS check in nth_value: windowContext.validate() already throws for any RESPECT/IGNORE NULLS usage because supportNullsDesc() returns false. - Migrate 19 positive-result tests from assertSql to assertQueryNoLeakCheck to validate factory cursor properties (supportsRandomAccess, expectSize, expectedTimestamp). - Replace the -1 position assertions with concrete character offsets in 5 error-path tests so they verify error anchoring. - Add coverage: non-constant, negative, and NULL arguments for ntile and nth_value; ntile without ORDER BY; cume_dist DESC ordering, multi-column ORDER BY, partitioned peers; EXPLAIN plan tests for all three functions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
jovfer
marked this pull request as ready for review
April 15, 2026 18:50
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
- Strengthen weak tests that passed trivially: testCumeDistSingleRow renamed to testCumeDistTwoRows and asserts distinct 0.5/1.0 outputs so a hardcoded 1.0 return would fail. testNthValueWithNulls adds a second nth_value(val, 2) assertion so the n=1 NULL output is differentiated from the n=2 20.0 output. testNthValueBeyondFrameSize asserts n=2 (resolves), n=3 (boundary), n=10 (far beyond) in one query to pin the n<=totalRows threshold. - Add coverage for multi-key PARTITION BY (testNthValuePartitioned- ByTwoColumns) and for the single-peer-group path in cume_dist (testCumeDistAllPeers). - Reword nth_value error messages from "nth_value n must be..." to "n must be..." to match the lag/lead offset-error convention. - Fix inconsistent ZERO_PASS qualifier (missed in the prior pass). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Add tests that target previously uncovered branches in NthValueDoubleWindowFunctionFactory and CumeDistFunctionFactory: - testNthValueWholeResultSet: nth_value(val, n) over () — the no-partition + no-order + default-frame path, previously 0%. - testNthValuePartitionedRowsFrameBounded: partitioned ROWS frame with rowsLo > MIN_VALUE and rowsHi < 0, which routes to NthValueOverPartitionRowsFrameFunction (previously 19%). - testNthValueRowsFrameExcludingCurrent / Unbounded-to-preceding: strengthen NthValueOverRowsFrameFunction for frames that do not include the current row. - testNthValueRangeUnboundedToPreceding: RANGE frame with unbounded preceding + non-zero hi, exercises RangeFrameFunction. - testNthValueRangeFrameBufferExpansion: shrinks the window store page size so large partitions force expandRingBuffer in both partitioned and non-partitioned RANGE frames. - testCumeDistPartitionedNoOrder: cume_dist() over (partition by i) — CumeDistNoOrderFunction was only hit by the non-partitioned case before. Overall new-line coverage for the three factories improves from 67.83% to ~76%, with NthValue specifically going from 58% to 71%. Also realign the Javadoc @param column in NtileFunctionFactory so the IntelliJ formatter leaves it alone — the CI "Applying formatting" step was rewriting these two lines and failing the "git diff --exit-code" gate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three new tests target computeNext branches that the earlier set missed, and the existing plan tests are extended to cover every factory routing branch: - testNthValuePartitionedRowsUnboundedLockedIn exercises the partitioned ROWS frame "frame locked in" early return at NthValueOverPartitionRowsFrameFunction.computeNext:717-722 by using rowsLo=MIN_VALUE with n=1. - testNthValuePartitionedRowsIncludingCurrent uses rows between 2 preceding and current row to hit the frameIncludesCurrentValue branch at lines 748-758, including the n == currentFrameSize + 1 case that returns the current row's own value. - testNthValueRowsFrameIncludingCurrentRow bundles three non-partitioned assertions — n=3 and n=5 on rows between 2 preceding and current row (lines 1121, 1123), and n=1 on rows between unbounded preceding and 2 preceding for the non-partitioned "frame locked in" path (lines 1109-1111). Make testNthValueRangeUnboundedToPreceding deterministic across both MICRO and NANO timestamp modes. The previous version used 1-unit ts spacing, so "2 microseconds preceding" produced different semantics between modes (random test seed picks the mode). New version uses 1s spacing and branches the expected text on timestampType. Extend testCumeDistToPlan and testNthValueToPlan with assertPlanNoLeakCheck calls for every remaining factory branch — the original tests covered 3 classes, the new set covers all 12 (3 CumeDist plus 9 NthValue inner classes). RANGE plans use the same MICRO/NANO parameterization as testStdDevPopToPlan. Overall new-line coverage for the three factories moves from 67.83% to ~86% after these additions. The largest jumps are in NthValueOverPartitionRowsFrameFunction (19% -> 85%) and NthValueOverWholeResultSetFunction (0% -> 62%). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
NthValueOverPartitionFunction and NthValueOverWholeResultSetFunction declare TWO_PASS via getPassCount(), which forces the engine into the CachedWindowRecordCursorFactory path — that factory only calls pass1/pass2 and never calls computeNext or getDouble on the function. The interface defaults (empty computeNext and a throwing getDouble) are sufficient for TWO_PASS functions. Remove the hand-written overrides, matching the convention in AvgOverPartition- Function and AvgOverWholeResultSetFunction. This also removes the NthValueOverPartitionFunction.nthValue field, which was never assigned — getDouble returned the default NaN. No behaviour change. Coverage percentages for the two affected classes rise because the previously uncovered method bodies no longer exist. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two correctness bugs in NthValueOverRowsFrameFunction and NthValueOverPartitionRowsFrameFunction produced wrong results for frames that exclude recent rows: Bug A — rows between unbounded preceding and K preceding: the ring buffer of size K could not retain row n's value once more than K rows had been processed, causing nth_value(n) to rotate through stale buffer positions instead of locking onto the correct permanent value. Fix replaces the ring-buffer approach with a simple (totalCount, lockedValue) state machine that captures row n's value once and emits it after the frame grows large enough. O(1) memory per partition. Bug B — rows between X preceding and Y preceding (Y > 0): the frame-element count formula min(effectiveCount, frameSize) did not subtract the Y-preceding exclusion zone, so nth_value read past the frame boundary for the first few rows. Fix uses count + 1 - excludeCount for the non-FIC (frame-includes- current) case, which correctly delays frame-element counting until the excluded tail has passed. Both the non-partitioned and partitioned ROWS-frame classes carry the same fix. The RANGE-frame variants do not have the analogous bugs because their early-return path freezes all state (including the buffer pointer) and their frame-size computation is timestamp-based rather than counter-based. Three tests that previously locked in the wrong behaviour now assert SQL-standard-correct values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The !frameLoBounded arm of NthValueOverRangeFrameFunction and its partitioned sibling grew frameSize by at most one per incoming row, inspecting only the element at firstIdx. For RANGE between unbounded preceding and K preceding this miscounts whenever multiple previously- ineligible rows cross the minDiff threshold in the same step (e.g. a cluster of tightly-spaced rows followed by a larger gap): the function returns NaN for the first rows whose frame should already contain n elements. firstIdx never retreats in unbounded-lo mode, so the fix is a simple forward scan from frameSize to size, mirroring the shape of the frameLoBounded branch. Also close review gaps: regression tests that pin the earlier ROWS frame fixes (rows between unbounded preceding and K preceding, and rows between X preceding and Y preceding), factory-routing coverage for ROWS unbounded..current, ROWS unbounded..unbounded and RANGE unbounded..current, plus IGNORE NULLS rejection for ntile/cume_dist and the empty-frame DoubleNullFunction fallback for nth_value. Stale line-number comments on two existing tests are replaced with symbolic descriptions that survive future refactors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CumeDistOverPartitionFunction previously kept its deferred-offsets list in ObjList<LongList> with one LongList per partition, allocated on the first row of each partition in pass1. At high partition cardinality that is a per-partition Java allocation on the data path, against the zero-GC contract that other window functions in the package follow. Switch to the same shape used by NthValueOverPartitionRangeFrameFunction and the first_value / last_value / avg / min / max families: a single shared MemoryARW backs deferred offsets for all partitions, each partition owns a slice identified by (startOffset, size, capacity) stored in the map value, and slices grow through expandRingBuffer with the shared freeList recycling retired blocks. No per-partition Java objects; one LongList per factory instance (freeList) and one MemoryARW per factory instance. Split the nth_value ROWS frame classes so the unbounded-preceding..K preceding case does not allocate a MemoryARW it never touches. NthValueOverPartitionRowsFrameFunction and NthValueOverRowsFrameFunction now handle the bounded-lo case exclusively (frameLoBounded is always true, the dual-layout map-slot scheme is gone). Two new classes NthValueOverPartitionRowsFrameUnboundedFunction and NthValueOverRowsFrameUnboundedFunction carry the O(1) state machine (count + lockedValue) and no buffer. The factory routes based on rowsLo == Long.MIN_VALUE before choosing which class to build. All 418 WindowFunctionTest tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds plan assertions for the two nth_value classes whose toPlan() was previously at 0% coverage: - NthValueOverRowsFrameUnboundedFunction (non-partitioned ROWS BETWEEN UNBOUNDED PRECEDING AND K PRECEDING) - NthValueOverPartitionRowsFrameUnboundedFunction (partitioned variant of the same frame shape) The functions themselves were already exercised by computeNext tests (testNthValueRowsFrameIncludingCurrentRow and testNthValuePartitionedRowsUnboundedLockedIn), but their plan output was never asserted, so the toPlan() text could regress unnoticed. Coverage rationale: the remaining 0% methods on these and other nth_value classes are pass1 (dead under ZERO_PASS executor routing, kept as WindowFunction interface boilerplate matching first_value / last_value), getName (the factory-level NAME constant is used in the hot path), and toTop on classes routed through CachedWindow (whose toTop only rewinds the record chain, not the per-function state). None of those represent reachable behaviour worth testing at this layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Critical - ntile() and cume_dist() now reject EXCLUDE clauses. The previous isDefaultFrame() check did not inspect the EXCLUDE clause, so a user-written default-shape frame with EXCLUDE GROUP / EXCLUDE TIES silently dropped the modifier and produced wrong results. - nth_value RANGE-with-CURRENT-ROW comment now points at the project- wide non-peer-expanding convention shared by sum/avg/min/max/ first_value/last_value (35+ sites). The previous comment claimed "the flag only affects EXPLAIN output," which misled readers about the divergence from SQL standard semantics on tied ORDER BY values. Correctness - Reject ROWS frame bounds with |rowsLo| > Integer.MAX_VALUE up front. bufferSize = (int) Math.abs(rowsLo) silently produced negative ints for very large bounds, collapsing the ring-buffer math. - Route the no-partition NthValueOverRangeFrameFunction through the shared expandRingBuffer + freeList path. The previous inline capacity-doubling never reclaimed the old chunks via MemoryARW freelist, leaking up to ~16 MiB of native memory per cursor lifetime on a 1M-row no-partition query. - Document reset()/reopen() asymmetry. The native memory close-then- lazy-reallocate pattern matches FirstValue/LastValue/Avg and is required by the cursor RSS budget; switching to truncate() breaks the post-close memory check. User-visible - nth_value toPlan now emits "range between unbounded preceding and K preceding" correctly for UNBOUNDED+K-PRECEDING RANGE frames. The previous output collapsed to "K preceding and K preceding". - INT-typed NULL n now reports "n cannot be NULL" instead of "n must be a positive integer". Same fix applied to ntile bucket count. - ntile error messages now consistently prefix with "ntile()". Tests - Added EXCLUDE rejection tests for ntile / cume_dist (all three EXCLUDE_* modes) and EXCLUDE_GROUP / EXCLUDE_TIES rejection for nth_value via windowContext.validate(). - Added FOLLOWING and GROUPS rejection tests for nth_value (it lives in WINDOW_ONLY_FUNCTIONS so the FRAME_FUNCTIONS parametric loops do not cover it). - Added arity test for nth_value(val). - Added combined-select test exercising ntile + cume_dist + nth_value in one query. - Switched testNtileNullPartitionKey to ntile(3) with asymmetric data so the assertion actually distinguishes "NULLs as own partition" from "NULLs merged". - Converted nine assertSql() call sites to assertQueryNoLeakCheck() per CLAUDE.md, with explicit factory-property arguments where the outer aggregation diverges from defaults. Deferred (filed as project-wide follow-ups) - toPlan ORDER BY emission. Would diverge from FirstValue/LastValue/ Avg/Min/Max which all omit it. - pass1/pass2 short-circuit on partitioned ntile/nth_value (perf). - cume_dist 1.0 placeholder write (perf). - expandRingBuffer O(P) freelist scan (pre-existing, perf). - NaN-fill loop -> Vect.memset (perf). - cume_dist / percent_rank pass1 dedup (refactor).
- Replace the "lookup always hits" comment in NthValueOverPartitionFunction.pass2 with an explicit assert. The contract still holds (pass1 populates an entry for every partition key seen by pass2), but a future change to the cursor stream would now trip the assert in -ea builds instead of silently NPEing. - Add testNthValueRejectsFrameOverflow covering both branches of the int-size guard on ROWS frame bounds (rowsLo and rowsHi separately). - Add testNtileOrderByNullColumn, mirroring the existing cume_dist NULL-order test for the ntile factory. - Add testNtilePartitionedReopen, mirroring the cume_dist partitioned reopen sibling so that the ntile per-partition map lifecycle is exercised by an assertQueryNoLeakCheck reopen path. - Add testNthValuePartitionedRangeFrameBoundedWithNulls covering the frameLoBounded=true variant of NthValueOverPartitionRangeFrameFunction with a NULL value landing at frame index n-1 inside the native ring buffer. - Rename testNtileRejectsExcludeCurrentRow / testCumeDistRejectsExcludeCurrentRow to *RejectsExcludeClause; both methods already assert all three EXCLUDE modes, so the new name reflects actual coverage.
Address review-flagged coverage gaps for the three new window functions. Each new test exercises a code path not previously covered: - testNthValueRowsExcludeCurrentRow: confirms the EXCLUDE CURRENT ROW clause on a ROWS frame normalises rowsHi from 0 to -1 and keeps the current row out of nth_value's frame. - testNthValueRangeFrameOrderByDesc: drives the descending-comparator path through NthValueOverRangeFrameFunction, which previous tests reached only via ASC ordering. - testNtilePartitionedOrderByDesc / testCumeDistPartitionedOrderByDesc: combine PARTITION BY with ORDER BY ts DESC, which the existing DESC tests covered only without a partition key. - testNtilePartitionedByTwoColumns / testCumeDistPartitionedByTwoColumns: exercise composite partition keys for the two new factories; only nth_value had a two-column partition test before. - testNtilePartitionedBySymbol / testNthValuePartitionedBySymbol: cover SYMBOL-typed partition keys, which take a different RecordSink path than LONG keys. - testNthValueRowsNEqualsPartitionSize: pins the count == n boundary where the locking write fires on the very last row of each partition; previous tests only ran with n strictly inside or beyond the partition size. - testNtileCumeDistNthValueNamedWindow: drives all three factories through a single WINDOW w AS (...) clause, exercising the named- window resolution path. - testNtileCumeDistNthValueOverFilteredCursor: composes the three factories on top of a WHERE-filtered subquery so the window cursor only sees surviving rows.
CumeDistOverPartitionFunction.pass2 now asserts the partition map entry exists, matching the invariant already pinned by NthValueOverPartitionFunction.pass2. Without the assertion an unreachable null deref would silently NPE under -da. CumeDistFunction.toTop/reset/reopen now also reset rank and lastRecordOffset. The "first row sets derived state" pattern still holds, but explicit resets remove the coupling so future changes to pass1 cannot accidentally read stale state. CumeDistOverPartitionFunction.preparePass2 now obtains the map record via map.getRecord(), aligning with the shape used by AvgDoubleWindowFunctionFactory.preparePass2. The unbounded-preceding nth_value variants (with and without PARTITION BY) now assert rowsHi != Long.MIN_VALUE in addition to rowsHi < 0, preventing a latent (int) Math.abs overflow if the parser ever surfaced MIN_VALUE on the upper bound. Added three regression tests: - testNthValueAcceptsExcludeNoOthers pins EXCLUDE NO OTHERS as a no-op accept path through windowContext.validate(). - testNthValueAcceptsConstantExpression and testNtileAcceptsConstantExpression pin constant-folded n / bucket count arguments (e.g. ntile(1+1), nth_value(val, 1+1)). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/io/questdb/griffin/engine/functions/window/NthValueDoubleWindowFunctionFactory.java`:
- Around line 723-741: The toPlan(PlanSink sink) implementation for
NthValueDoubleWindowFunctionFactory currently prints only PARTITION BY and frame
bounds (using arg, n, partitionByRecord.getFunctions(), frameLoBounded, maxDiff,
minDiff) and omits ORDER BY for ordered zero-pass nth_value() variants; update
toPlan to detect and render the ORDER BY clause (same pattern used elsewhere for
ordered RANGE/ROWS variants) by appending the order-by expressions and
directions after partitionByRecord.getFunctions() and before the frame text so
the plan shows the full window spec; apply the same fix to the other ordered
nth_value() toPlan branches mentioned (lines covering the other variants) so all
ordered variants include ORDER BY in their EXPLAIN output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d10ae13a-6c98-48fb-88a9-1dfe55f13efa
📒 Files selected for processing (6)
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.javacore/src/main/java/io/questdb/griffin/engine/functions/window/CumeDistFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/window/NthValueDoubleWindowFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/window/NtileFunctionFactory.javacore/src/main/resources/function_list.txtcore/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java
Targets coverage gaps surfaced by jacoco against
NthValueDoubleWindowFunctionFactory:
- testNthValueToPlan now also EXPLAINs four shapes whose toPlan branches
were skipped by the existing cases:
- RANGE between unbounded preceding and K preceding (hits the
"unbounded preceding" lo-bound and "<minDiff> preceding" hi-bound
sinks of NthValueOverRangeFrameFunction.toPlan)
- the partitioned analogue
(NthValueOverPartitionRangeFrameFunction.toPlan)
- ROWS between K preceding and M preceding (excludeCount > 0) on
NthValueOverRowsFrameFunction.toPlan
- the partitioned analogue
(NthValueOverPartitionRowsFrameFunction.toPlan)
- testNthValuePartitionedRangeUnboundedBufferExpansion is new. It
forces NthValueOverPartitionRangeFrameFunction.computeNext to grow
its per-partition ring buffer past the default initial capacity by
scanning 80 rows per partition through a 1000-second bounded RANGE
window with n=50, so the size==capacity path executes and
expandRingBuffer fires. This complements
testNthValueRangeFrameBufferExpansion, which only reliably grows
the non-partitioned buffer because the partitioned override path
was not actually triggered by its property knobs under the current
config-propagation path.
Local jacoco focused on WindowFunctionTest drops the nth_value
factory's uncovered/partial-line count from ~131 to 91 (-40 lines).
Remaining uncovered lines are dominated by allocator-failure
try/catch blocks, getName() returning the NAME constant, and
ZERO_PASS pass1 bodies that the engine bypasses through the
getDouble path; lifting those would need a fault-injecting memory
factory or engine changes outside the scope of a coverage polish.
The cume_dist and ntile factories' residual uncovered lines are in
similar territory (allocator cleanup, getName, plus toTop methods
that current dispatch routes to CachedWindow and never invokes).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The streaming WindowRecordCursorFactory drives every all-ZERO_PASS query through computeNext + getDouble and never invokes pass1; the only dispatch path that calls pass1 is CachedWindowRecordCursorFactory, which SqlCodeGenerator picks when at least one window column is non-ZERO_PASS (or an extra sort is needed). With every prior test selecting nth_value in isolation, pass1 of the eight ZERO_PASS nth_value variants and CumeDistNoOrderFunction were dead lines in coverage terms even though they are the values-producing path in any real mixed-window query. The new test pairs cume_dist over ORDER BY (TWO_PASS) plus cume_dist over PARTITION BY (no order, ZERO_PASS) with one nth_value column for every ZERO_PASS frame shape: current row, partitioned RANGE bounded, partitioned ROWS bounded, partitioned ROWS unbounded preceding to K preceding, non-partitioned RANGE bounded, non-partitioned ROWS bounded, non-partitioned ROWS unbounded preceding to K preceding, and the partitioned unbounded-preceding-to-current variant. The presence of the TWO_PASS column downgrades the whole SELECT to CachedWindow, exercising pass1 on every function in the chain build. Local jacoco confirms 24 nth_value pass1 lines and the 2-line CumeDistNoOrderFunction.pass1 body flip from nc to fc. Residual uncovered nth_value lines drop from 91 to 67; cume_dist drops from 30 to 28. NtileFunctionFactory is unchanged because both its pass1 bodies are already exercised through its native TWO_PASS dispatch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NtileOverPartitionFunction.pass2 calls map.findValue() and then dereferences the returned MapValue to read the partition's total row count. The sibling pass2 implementations on the same dispatch path -- CumeDistOverPartitionFunction.pass2 and NthValueOverPartitionFunction.pass2 -- include an assert mapValue != null immediately after findValue to catch any divergence between pass1 and pass2 partition keys early. Add the same assert to ntile so the three siblings stay in sync; without it, an invariant break would surface only as an opaque NPE on the next mapValue.getLong(0) line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
testPercentRankExplainPlan previously covered only the dismissOrder=true paths -- OVER (), OVER (PARTITION BY s), OVER (ORDER BY ts) and OVER (PARTITION BY s ORDER BY ts) -- so the latent NPE in PercentRankFunction.toPlan and PercentRankOverPartitionFunction.toPlan that fires when orderBy is null was never test-caught. The recent fix in SqlCodeGenerator (passing ac.getOrderBy() to initRecordComparator on the grouped-comparator path) silently closed that hole, but the percent_rank() side gained no regression coverage. Add two assertPlanNoLeakCheck cases that route through the osz > 0 && !dismissOrder branch: - percent_rank() OVER (ORDER BY i) for PercentRankFunction.toPlan - percent_rank() OVER (PARTITION BY s ORDER BY i) for PercentRankOverPartitionFunction.toPlan This mirrors the new testCumeDistExplainPlan and testNtileExplainPlan in WindowFunctionTest, so all three TWO_PASS factories have parity coverage on both dismissOrder paths. Reverting the SqlCodeGenerator fix locally reproduces the original NPE on these new cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… sm_ntile_cumedist_nthvalue # Conflicts: # core/src/test/java/io/questdb/test/griffin/engine/window/PercentRankFunctionTest.java
bluestreak01
approved these changes
May 5, 2026
Contributor
[PR Coverage check]😍 pass : 1106 / 1192 (92.79%) file detail
|
3 tasks
javier
added a commit
to questdb/documentation
that referenced
this pull request
May 15, 2026
## Summary Documents the three new window functions added in questdb/questdb#6925: - **`ntile(n)`** — distributes rows of an ordered partition into `n` approximately equal buckets and returns the 1-based bucket number. - **`cume_dist()`** — returns the cumulative distribution of the current row (including peers) within the partition. - **`nth_value(value, n)`** — returns the `n`-th value (1-based) within the current window frame, or `NULL` when the frame is smaller than `n`. Accepts `double`, `long`, and `timestamp` values. Each section calls out the actual rules from the PR — `n` must be a constant, framing is rejected for `ntile` / `cume_dist`, `nth_value` rejects `IGNORE NULLS` / `RESPECT NULLS` / `FROM FIRST` / `FROM LAST`, etc. Tabular examples mirror the test data in `WindowFunctionTest`. Also: - Updated the overview quick-reference table with the three functions. - Updated the ranking-functions section header (these new ranking-style functions don't all return ranks, so the header now says "ranks, row numbers, or partition-scoped distribution values"). - Removed the obsolete "`ntile()` and `cume_dist()` are not currently supported" line from the bottom Notes. ## Test plan - [x] `yarn build` succeeds - [x] Visual review of `query/functions/window-functions/overview.md` and `query/functions/window-functions/reference.md` - [x] Anchor links `#nth_value`, `#cume_dist`, `#ntile` resolve from the overview quick-reference table 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: javier <javier@formatinternet.com> Co-authored-by: javier ramírez <javier@questdb.io> Co-authored-by: Vlad Ilyushchenko <bluestreak01@users.noreply.github.com> Co-authored-by: Sergey Minaev <5072859+jovfer@users.noreply.github.com>
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
3 participants
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.
Addresses questdb/roadmap#120 (Gap 2).
This PR covers only Gap 2 and does not close the roadmap issue.
Summary
ntile(n)window function — distributes rows of an ordered partition intonapproximately equal buckets and returns the 1-based bucket number.cume_dist()window function — returns the cumulative distribution:rows at or before the current row (including peers) divided by total rows in the partition.
nth_value(expr, n)window function — returns then-th value (1-based) within the current window frame, or NULL whennexceeds the frame size. The argument must beDOUBLEin this PR;LONGandTIMESTAMPoverloads are deferred to a follow-up that will mirror the existing per-type factory split used byfirst_value/last_value(*Double,*Long,*Timestamp).PARTITION BYandORDER BY.nth_valuesupportsROWSandRANGEframes (bounded and unbounded).Side effect
SqlCodeGeneratornow forwards theORDER BYexpression list toWindowFunction.initRecordComparatoron the grouped-comparator path. This was previouslynull, which leftpercent_rank()'s EXPLAIN output incomplete on non-timestampORDER BY. The new functions need the list for their plan rendering, and threading it through also tightens the existingpercent_rankplan output.Tradeoffs / limitations
ntileandcume_distrun in two passes and maintain per-partition state in aMap; memory scales with partition cardinality.cume_distpartitioned: per-partition deferred-offset slices live in a single shared native buffer and are not released until the cursor closes. Worst case is O(total rows) of native memory when peer groups span entire partitions (e.g.ORDER BYa constant column).ntileandcume_distreject explicitROWS/RANGE/GROUPSframe clauses — they are always partition-scoped.nth_valueaccepts onlyDOUBLEfirst argument in this PR;LONGandTIMESTAMPoverloads will follow.nth_valuerequiresnto be a compile-time constant; a non-constant expression fornis rejected.nth_valuerejectsIGNORE NULLS/RESPECT NULLSand does not supportFROM FIRST/FROM LAST.nth_valueRANGE ... CURRENT ROWfollows the project-wide QuestDB convention shared withsum/avg/min/max/first_value/last_value: a frame ending atCURRENT ROWdoes not look ahead to peer rows that share the sameORDER BYvalue. This diverges from the SQL standard / PostgreSQL on tiedORDER BYvalues; revisiting peer semantics is tracked as a project-wide follow-up that should cover allRANGE-supporting window factories together.Related PRs
nth_value(<long>, n)andnth_value(<timestamp>, n)overloads on top of this branch.Test plan
|rowsLo|/|rowsHi|>Integer.MAX_VALUE)🤖 Generated with Claude Code