Add SQL histogram and date_histogram bucket functions - #5700
Conversation
PR Reviewer Guide 🔍(Review updated until commit cc420ab)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 6573786
Previous suggestionsSuggestions up to commit 80f8b16
|
80f8b16 to
7151095
Compare
|
Persistent review updated to latest commit 7151095 |
Adds parse-time support for `histogram` and `date_histogram` in V2 SQL with
named-argument invocation. Each call is lowered during AST construction to
primitives that already exist -- `Span`, `COALESCE`, `DATE_FORMAT`,
`TIMESTAMPADD` -- so no new engine function or execution operator is
introduced, and the lowering happens before the V2 and analytics-engine paths
diverge.
Supported parameters:
histogram field, interval, offset, missing
date_histogram field, interval / fixed_interval / calendar_interval,
format, time_zone, missing
`min_doc_count`, `order` and `alias` are rejected: they would have to mutate
the surrounding query (HAVING / ORDER BY / the SELECT-list alias), which needs
parser plumbing that reaches outside the function call. `date_histogram`'s
`offset` is rejected pending a duration-string parser distinct from
`time_zone`'s ZoneOffset format.
These functions are new to the V2 grammar but not to the plugin, and that is
where the care is needed. The legacy engine has accepted
`date_histogram(field=<col>, 'interval'=<n>)` in GROUP BY since before V2
existed, and requests reach it only when V2 raises SyntaxCheckException -- the
only type RestSQLQueryAction falls back on. Teaching V2 to match those calls
means it answers them first, so declining an unrecognized call shape with
SemanticCheckException would stop the query at V2 and silently drop a working
feature. Measured on a live cluster, `SELECT COUNT(*) FROM idx GROUP BY
date_histogram(field='ts','interval'='1h')` returned four buckets before the
grammar change and HTTP 400 after it.
Both expanders therefore decline an unrecognized shape with
SyntaxCheckException. Every other rejection is unchanged on purpose: once a
call is in the property-bag form these expanders own, a bad parameter is the
caller's mistake, and handing it to an engine that never understood the query
would answer a clear error with a confusing one.
The expander unit tests assert the shape of the AST that gets built, which says
nothing about whether the lowered Span survives analysis, planning and
pushdown. DateHistogramBucketFunctionIT asserts bucket keys and counts against
date_histogram_test, 72 documents on fixed timestamps chosen so an hourly
grouping must yield 12/24/17/19 and a half-hourly one 5/7/11/13/17/19. It
covers hourly, half-hourly and daily intervals, the fixed_interval and
calendar_interval synonyms, a second grouping key, a WHERE clause, numeric
histogram buckets, and both positional forms still reaching the legacy engine.
One test records a limitation rather than a guarantee. Selecting the bucket
alongside a second grouping key directly off the table leaves the span's field
typed UNDEFINED by the time the aggregate runs and the request fails; wrapping
the scan in its own derived table resolves it, and a single grouping key is
unaffected either way. Clients already emit the wrapped form, so this is pinned
where it can be seen rather than left as folklore in a comment.
Co-authored-by: Varun <stvarun11@gmail.com>
Signed-off-by: Jialiang Liang <ryanleeang@gmail.com>
7151095 to
6573786
Compare
|
Persistent review updated to latest commit 6573786 |
CsvFormatResponseIT.dateHistogramTest has been asserting this query for years:
SELECT COUNT(*) FROM <idx>
GROUP BY date_histogram('field'='insert_time','fixed_interval'='4d','alias'='days')
It broke once these names entered the V2 grammar. The keys are quoted, so V2
reads it as named arguments and takes over, then rejects `alias` -- a parameter
the legacy engine implements and this expander does not.
The earlier fix assumed the quoted-key form belongs to V2, so a bad parameter
there is the caller's error. That is wrong: legacy uses the same spelling and
accepts parameters V2 has no lowering for, so "unsupported here" cannot be
treated as "invalid". Every rejection in the bucket package now raises
SyntaxCheckException, which means anything this expander cannot lower reaches
the legacy engine exactly as it did before the grammar change -- answered if
legacy understands it, and refused with legacy's own message if not. The cost
is that a genuine typo in the V2 form gets legacy's error rather than ours;
that is worth far less than a query that used to work.
Adds coverage for the `alias` case at both levels, since the positional form
alone did not catch it.
Signed-off-by: Jialiang Liang <ryanleeang@gmail.com>
|
Persistent review updated to latest commit cc420ab |
…cs engine Verified against a local analytics-engine sandbox (9 plugins, every index parquet-backed so all data queries route to DataFusion). Three problems showed up, none of them visible on the default route. The dataset could not load at all. Parquet-backed indices are append-only and reject a custom document id, so all 72 bulk items failed and every assertion saw an empty index. The ids were never read by any test; dropping them lets the same dataset load on both routes. Three tests asserted results that only the legacy engine can produce. The old `date_histogram(field=<col>, ...)` spelling, and the `alias` parameter, are understood only by the legacy V1 engine, and that engine is reachable only through RestSQLQueryAction -- the analytics route enters through RestUnifiedQueryAction, which has no fallback to it. Those queries have never worked on the analytics route, before or after this change, so tests asserting their results can only ever pass on one of the two. Removed. The behaviour they guarded is still covered where it belongs: CsvFormatResponseIT.dateHistogramTest has asserted the `alias` shape for years and is what caught the regression in CI, and the expander unit tests assert the exception type directly, without needing an engine at all. One test asserted a failure -- that a second grouping key over a bare table scan leaves the span's field typed UNDEFINED. That is a V2 execution defect, not a property of these functions, and the analytics route resolves the same query correctly. Pinning it made the suite demand an engine bug stay unfixed and fail wherever it was already fixed. Removed; the constraint is noted on the test that uses the derived-table form. Seven tests remain, all asserting what a query returns rather than which engine answered it. They pass identically on both routes. Signed-off-by: Jialiang Liang <ryanleeang@gmail.com>
…ping them Three of these tests assert results only the legacy V1 engine can produce: the positional `date_histogram(field=<col>, ...)` spelling and the `alias` parameter. That engine is reachable only through RestSQLQueryAction's SyntaxCheckException fallback, and the analytics-engine route enters through RestUnifiedQueryAction, which has no such fallback -- so those queries have never worked there. They were removed in the previous commit to keep the suite green on both routes. Restoring them behind @RequiresCapability keeps the guard where it matters and still leaves both routes green, which is what the existing capability mechanism is for: the default route runs all ten, the analytics route skips these three with the reason printed. The guard is worth keeping -- these are the shapes a V2 grammar addition can silently take away from the legacy engine, which is exactly the regression CI caught here. LEGACY_ENGINE_FALLBACK is worded after LEGACY_METHOD_QUERY, which covers the same situation for method-query syntax. Signed-off-by: Jialiang Liang <ryanleeang@gmail.com>
Test report — with and without the analytics engineVerified locally on both routes at This PR's tests
The seven that run on both routes return identical values: hourly The three skipped ones assert results only the legacy V1 engine produces — the positional Full suite on the analytics engineWhole
The 13 and the 15 are the same kind of test — Also fixedThe dataset carried explicit document ids. Parquet-backed indices are append-only and reject them, so all 72 bulk items failed and every assertion saw an empty index. Dropping the ids lets one dataset serve both routes. Harness notesFrom dai-chen/sql-1
|
1 participant
Description
Adds
histogramanddate_histogramto V2 SQL as bucket functions. Each call is lowered during AST construction to primitives that already exist (Span,COALESCE,DATE_FORMAT,TIMESTAMPADD), so no new engine function or execution operator is introduced.Usage
Arguments are named. Compute the bucket in a subquery and group by its alias — the planner does not accept
GROUP BY <expression>directly.{ "schema": [ { "name": "b", "type": "timestamp" }, { "name": "COUNT(*)", "type": "long" } ], "datarows": [ ["2026-01-01 00:00:00", 12], ["2026-01-01 01:00:00", 24], ["2026-01-01 02:00:00", 17], ["2026-01-01 03:00:00", 19] ], "total": 4, "size": 4, "status": 200 }The bucket comes back as a
timestamp, so intervals below an hour split as you would expect, and a second grouping key works alongside it:histogrambuckets a numeric field the same way and returns the bucket's lower bound:Parameters
histogramfield,interval,offset,missingdate_histogramfield,interval/fixed_interval/calendar_interval,format,time_zone,missingThe three interval spellings are synonyms; exactly one must be present.
min_doc_count,orderandaliasare rejected because they would have to mutate the surrounding query (HAVING / ORDER BY / the SELECT-list alias).date_histogram'soffsetis rejected pending a duration-string parser distinct fromtime_zone'sZoneOffsetformat.Positional calls keep going to the legacy engine
These names are new to the V2 grammar but not to the plugin — the legacy engine has accepted
date_histogram(field=<col>, 'interval'=<n>)inGROUP BYfor a long time, and queries reach it only when V2 raisesSyntaxCheckException, the one exceptionRestSQLQueryActionfalls back on. Now that V2 matches these calls first, an unrecognized shape has to decline with that exception or the query stops at V2:GROUP BY date_histogram(field='ts','interval'='1h')GROUP BY date_histogram('field'='ts','interval'='1h')Other rejections are unchanged: once a call is in the named-argument form, a bad parameter is the caller's error and gets a clear message instead of being re-run by an engine that never understood the query.
Check List
--signoff.Not yet verified on the analytics-engine route; draft until it is.