Repository navigation
fix(search): route un-LIMITed row queries through scroll so SELECT returns every row (#209) - #210
Merged
Merged
Conversation
…turns every row (#209) A row query with no LIMIT executed through the one-shot search path emitted no top-level size, so Elasticsearch returned its default 10 hits silently - a 45-row index reported 10 arbitrary rows with HTTP 200. The gateway executor already routed plain projections to scroll, but the programmatic surface (search / searchAs / searchAsync) and the row queries the gateway predicate misses (windowed / script-field-only, whose `fields` is empty) all truncated. SingleSearch gains the canonical response-shape discriminators: windowRowQuery (the dispatch condition previously duplicated inline in search and searchAsync) and returnsRows (row-shaped vs aggregation-shaped). search/searchAsync now route no-LIMIT row-shaped queries through the scroll path - which pages completely (PIT / search_after, #197) and already handles window enrichment and script fields - collecting into the same ElasticResponse. Aggregation-shaped queries are never routed (their result is the aggregation, bounded by explicit terms sizes since #205/#207). The materializer is a JVM-shared daemonic ActorSystem, so it can never keep the JVM alive. CoreDqlExtension's licensed result cap widens with the routing: rule (2) now keys on returnsRows instead of fields.nonEmpty, so windowed and script-only no-LIMIT queries - which previously hit the "bounded" searchAsync branch and now return every row - are capped at maxQueryResults with the truncation flag like plain projections. Tests: ReturnsRowsSpec pins the shape discriminator on 8 statement shapes. New SelectCompletenessSpec (testkit + es6 rest/jest, es7 rest, es8/es9 java): 3-shard 45-doc index; plain, SELECT *, script-field-only and window-enriched projections without LIMIT return all 45 rows; an explicit LIMIT keeps its one-shot bound. Green on real ES 6.8/7.17/8.18/9.0. The script-field-only case is asserted via the raw search API: script_fields values come back wrapped in Elasticsearch's per-field array on every path - a pre-existing conversion defect surfaced by this 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 #209 — implements the decided option 2: route un-LIMITed SELECTs to the scroll path internally.
What
A row query with no
LIMITthrough the one-shot search path emitted no top-levelsize, soElasticsearch returned its default 10 hits silently (45-row index → 10 arbitrary rows,
HTTP 200, no flag). The gateway executor already routed plain projections to scroll — which is
why the REPL rarely showed it — but the programmatic surface (
search/searchAs/searchAsync)always truncated, and so did the row shapes the gateway predicate misses (windowed and
script-field-only projections, whose
SingleSearch.fieldsis empty).How
SingleSearch(sql) gains the canonical response-shape discriminators:windowRowQuery— the window-dispatch condition previously duplicated inline insearchand
searchAsync;returnsRows— row-shaped (plain / script-field / window-enriched) vs aggregation-shaped(GROUP BY / metric-only).
SearchApi(core) —searchandsearchAsyncroutelimit.isEmpty && returnsRowsqueries through the public
scroll(complete paging via PIT/search_after per PIT + search_after injects _shard_doc as primary sort, defeating doc-id skip on ES 8+ #197; windowenrichment and script fields already handled there), collecting rows into the same
ElasticResponse. Aggregation-shaped queries are never routed — their result is theaggregation itself, bounded by explicit
termssizes since GROUP BY without LIMIT silently returns only the top 10 groups (silent wrong answer) #205/Window PARTITION BY silently truncates to 10 partitions (same class as #205) #207. The materializer is aJVM-shared daemonic ActorSystem (cannot keep the JVM alive; clients don't own it). The
interception is a runtime
this match { case s: ScrollApi => … }so no trait signaturechanges reach downstream repos.
CoreDqlExtension(core) — the licensedmaxQueryResultscap (ADR D4 rule 2) widensfrom
fields.nonEmptytoreturnsRows: windowed/script-only no-LIMIT queries previously hitthe "ES-bounded" searchAsync branch; now that they return every row they are capped at the
quota with the truncation flag, exactly like plain projections. Without this, the routing
would have opened a quota bypass.
Tests
ReturnsRowsSpec(sql): pins the discriminator on 8 statement shapes (plain,SELECT *,script-only, windowed, metric-only, GROUP BY, GROUP BY+window, LIMIT-independence).
SelectCompletenessSpec(testkit + es6 rest, es6 jest, es7 rest, es8 java, es9 java):3-shard, 45-doc index (15 categories × 3, exact oracles):
SELECT *(viasearchAsUnchecked— thesearchAsmacro rejectsSELECT *at compile time), script-field-only, and window-enriched projections withoutLIMIT → all 45 rows;
LIMIT 7→ exactly 7 (one-shot preserved).(
GroupByCompletenessSpec,WindowPartitionCompletenessSpec) — 10/10.Found while validating (separate issue to propose)
script_fieldsvalues come back wrapped in Elasticsearch's per-field array on every path(
n -> List("CAT_02")):searchAsconversion of a script field to a scalar has always failed,and no existing test ever content-asserted a script-field value. Pre-existing, independent of
the routing; the spec asserts the script-only case via the raw
searchAPI and the defect willbe filed separately.
Notes
MultiSearch(UNION ALL) without LIMIT is out of scope (multi-index; scroll does not supportit) — unchanged behavior.
score()now ride scroll like the gateway already did forplain projections (no
_scorerelevance on the PIT/_docpath) — consistent with theestablished gateway semantics.
🤖 Generated with Claude Code