Repository navigation
fix(group-by): emit an explicit terms size so GROUP BY returns every group (#205) - #206
Merged
Merged
Conversation
…group (#205) GROUP BY with no LIMIT emitted no size on the terms aggregation, so Elasticsearch silently returned its default 10 buckets - a 100-category index reported 10 plausible-looking groups with no truncation flag. Bucket.update now defaults the bucket size to Bucket.DefaultSize (65536, Elasticsearch's own search.max_buckets ceiling) when the statement has no LIMIT, so the default matches SQL semantics and cardinalities beyond the ceiling fail loudly instead of truncating silently. An explicit LIMIT n still maps to size = n (unchanged top-N push-down). Also fixes the latent value-discard chain in the terms emission (bridge + hand-maintained es6 bridge): size, HAVING-derived include and exclude were each applied to the original termsAgg binding, so each assignment silently discarded the previous one - an exclude dropped the size just applied. New GroupByCompletenessSpec (testkit + es6 rest/jest, es7 rest, es8/es9 java): 3-shard index, 37 categories with exact per-group oracles; asserts group count, per-group counts and total doc coverage with no LIMIT, LIMIT push-down, and exact key-ordered top-N. Green on real ES 6.8/7.17/8.18/9.0. 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 #205.
What
SELECT category, COUNT(*) FROM t GROUP BY categoryon an index with 100 distinct categoriesreturned 10 rows — Elasticsearch's default
termsbucket size — with no error, no warning,and no truncation flag. In SQL,
GROUP BYwith noLIMITmeans every group.Fix (2 defects)
Bucket.update(sql/…/query/GroupBy.scala) — when the statement has noLIMIT, thebucket size is now
Bucket.DefaultSize = 65536(Elasticsearch's ownsearch.max_bucketsceiling) instead of
None. The generatedtermsaggregation always carries an explicitsize, so the default matches SQL semantics; a cardinality beyond 65536 fails loudlyserver-side (
too_many_buckets_exception) rather than truncating silently. An explicitLIMIT nstill maps tosize = n(the existing top-N push-down), unchanged.Terms-emission discard chain (
bridge/…/ElasticAggregation.scala, both the template and thehand-maintained es6 copy) — latent value-discard bug surfaced by (1):
size, HAVING-derivedinclude, andexcludewere each applied to the originaltermsAggbinding, so eachassignment silently discarded the previous one (an exclude dropped the size just applied; an
exclude also dropped an include). The steps now chain. This is why exclude-bearing GROUP BY
tests initially kept passing with no
size— the bug was masking the fix.Tests
SQLQuerySpec(template + es6): 18 expected JSONs updated per bridge — every GROUP BYtermsnow asserts
"size": 65536(or the LIMIT-derived size). 118/118 green on the template,es6, es7, es8 and es9 bridges;
sqlmodule 395/395 green.GroupByCompletenessSpec(testkit template + concrete subclasses for es6 rest, es6jest, es7 rest, es8 java, es9 java): 3-shard index, 37 categories (
cat_iholds exactlyidocs, so group count and per-group counts and total doc coverage are exact oracles):LIMIT→ 37 groups, every per-group count exact, sum = all 703 docs (the GROUP BY without LIMIT silently returns only the top 10 groups (silent wrong answer) #205 guard);LIMIT 5→ 5 groups (push-down preserved);ORDER BY category DESC LIMIT 3→ exact top keys/counts. Ordered on the key, not themetric: a metric-ordered terms top-N is shard-approximate by design (
doc_count_error) andthe first run of this spec actually caught it returning
cat_34instead ofcat_35.Not in this PR (follow-ups)
PARTITION BYhas the same defect (partitiontermsgets nosize; >10 partitionssilently lose window values). Deliberately not folded in — it interacts with the
top_hitscaps and the inline
OVER LIMITpush-down. Filed separately.sum_other_doc_count-based truncation warning (issue option 3) — defense-in-depth for thewill need the same
"size": 65536expectation update when they bump core.🤖 Generated with Claude Code