Repository navigation
fix(window): size the partition terms so PARTITION BY covers every partition (#207) - #208
Merged
Merged
Conversation
…rtition (#207) Window partition buckets were created with size = None and never pass Bucket.update, so the partition terms aggregation fell back to Elasticsearch's default 10 buckets - with more than 10 distinct partition values, rows in partitions 11+ silently got no window value (ranking windows lost those rows from the enrichment entirely). WindowFunction.buckets now creates partition buckets with Bucket.DefaultSize (65536, the search.max_buckets ceiling introduced by the #205 fix). The statement LIMIT keeps bounding base rows only - it never shrinks the partition count; the inline OVER LIMIT keeps bounding top_hits rows per partition. New WindowPartitionCompletenessSpec (testkit + es6 rest/jest, es7 rest, es8/es9 java): 3-shard index, 15 partitions with exact per-partition oracles; asserts ROW_NUMBER ranks and FIRST_VALUE enrichment across all 15 partitions. Green on real ES 6.8/7.17/8.18/9.0. Its first run also surfaced a pre-existing, separate truncation (plain SELECT without LIMIT returns ES's default 10 base hits) - documented in the spec, tracked separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
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.
Fixes #207.
What
Same silent-wrong-answer class as #205, on the window path: the
termsaggregation wrappingevery
OVER (PARTITION BY ...)window had no explicitsize, so Elasticsearch computed only itsdefault 10 partition buckets. With more than 10 distinct partition values, rows in partitions
11+ silently got no window value (
ROW_NUMBER/RANK/DENSE_RANK: those rows are missing fromthe ranking enrichment entirely;
FIRST_VALUE/LAST_VALUE/STDDEV/PERCENTILE_CONTOVER:enrichment
null).Fix
One line in the version-agnostic sql module —
WindowFunction.buckets(
sql/…/function/aggregate/package.scala) now creates partition buckets withSome(Bucket.DefaultSize)(the 65536 =search.max_bucketsceiling introduced by #205) insteadof
None. Partition buckets never passBucket.update(no statementLIMITapplies to thepartition count — the inline
OVER LIMITboundstop_hitsrows per partition, which isunchanged), so the size must be set at construction.
The
top_hitsinterplay that kept this out of PR #206 is bounded per partition, not per query:partition count is capped by actual data cardinality (and
search.max_buckets), and eachpartition still carries at most its existing
top_hitssize (100 default / inlineOVER LIMIT).The old behavior wasn't a smaller response — it was a wrong one.
Tests
SQLQuerySpec(template + es6): the 5 window expected JSONs per bridge now assert"size": 65536on the partition terms (4 ranking + the top-hits aggregation test). All fivebridges 118/118 (es9 under
sbt17— its elastic4s is Java-17 bytecode),sql395/395,core724/724, macros green, cross-compiled 2.12 + 2.13.WindowPartitionCompletenessSpec(testkit template + concrete subclasses for es6rest, es6 jest, es7 rest, es8 java, es9 java): 3-shard index, 15 partitions (
cat_iholds3 docs with amounts
i*100+1..3, exact per-partition oracles):ROW_NUMBER OVER (PARTITION BY category ORDER BY amount DESC)→ all 15 partitions ranked,ranks 1..3 each, rank-1 row is the per-partition max;
FIRST_VALUE(amount) OVER (...)→ every row of every partition enriched with the exactpartition max.
Existing window fixtures (emp: 5 departments) could never catch this — same cardinality-≤10
blind spot documented in #205.
Found while validating (filed as #209)
The spec's first live run exposed a third member of the truncation family, pre-existing and
out of scope here: a plain SELECT with no
LIMITon the non-scroll search path emits notop-level
size, so Elasticsearch returns its default 10 base hits (45-row index → 10arbitrary rows; the window enrichment itself was complete). Every row-count-asserting
WindowFunctionSpecquery carries an explicitLIMIT, which is why it never surfaced. The newspec uses
LIMIT 100with a comment pointing at that behavior, keeping the two truncationsseparate.
🤖 Generated with Claude Code