Conversation
The date picker appends a time filter to the query text, which the engine
only sees after it has already resolved the schema of the queried index
pattern -- it merges the mappings of every index the wildcard matches
before it parses the appended `where`. So it cannot use that filter to
skip indices holding no data in the picked range, and a pattern spanning
months of rollovers pays for all of them: their shards, their PIT
contexts, and their mapping conflicts, in a query that asks for 30
minutes.
Send the same bounds out of band so the engine has them up front:
{ "query": "...", "time_range": { "field": "@timestamp",
"from": "...", "to": "..." } }
The clause in the query text is unchanged, so results do not depend on
whether the engine reads this. Both come from FilterUtils
.getTimeFilterBounds, since a disagreement between the hint and the
clause could let the engine skip an index the filter would have matched.
Engines that do not read the field ignore it -- the PPL API parses its
body leniently -- so this is safe against older clusters.
Signed-off-by: Kai Huang <ahkcs@amazon.com>
The histogram query passes its own range rather than reading the global picker, and a dataset can have no time field at all. Pin both: the hint must describe the window the appended clause actually filters on, and be absent when no clause is appended. Signed-off-by: Kai Huang <ahkcs@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit b1e6c78)Here are some key observations to aid the review process:
|
Six fixes from review, in rough order of consequence. **A DST gap could shift a bound an hour.** formatTimePickerDate already emits `YYYY-MM-DD HH:mm:ss.SSS` in UTC; passing that back through formatDate re-parsed it as local time and re-emitted local components. A no-op except inside a spring-forward gap, where it moved the bound an hour -- `2026-03-08 02:30:00.000` becomes `03:30:00.000` in America/New_York -- dropping an hour of matching rows from the where clause. Both the clause and the bounds now use the values as they come, which also means they cannot describe different windows. (The same spurious wrap remains in sql_search_interceptor.ts:117 and is left for a separate change.) **An unparseable range produced garbage bounds.** A malformed range does not fail loudly: datemath yields '' when it parses to nothing, and a moment that formats as the literal 'Invalid date' when it parses to an invalid one. Both used to reach the wire (the former as 'NaN-aN-aN aN:aN:aN.NaN'). Bounds are now shape-checked and reported as absent instead, leaving the clause's existing behaviour alone. **The hint could reach an endpoint that rejects it.** Facet's body builder is shared with the async direct-query endpoints, whose parser throws on any field it does not know. Only S3 datasets take that path and they carry no time field, so the hint never actually got there -- an invariant one dataset-config change away from failing every S3 query. Restricted to the SQL/PPL actions. **The wire contract did not state its timezone.** The bounds are UTC wall clock with no designator; a consumer parsing them as local time would prune the wrong window. Documented on TimeRangeHint. **Two test gaps.** The drift guard only held for bare-string engines, so it never covered the TIMESTAMP() flavor that legacy Open Distro data sources use; it is now parameterized over both. The 'omits the hint' Facet test asserted the absence of a key nothing set, so it passed whether or not the guard existed; it now exercises the async endpoint. **A comment and test name claimed the histogram supplies its own time range.** It does not: the histogram derives its interval from the global timefilter and passes no range. The clause does prefer a caller-supplied range, so the hint must follow it, but nothing in this repo currently supplies one -- the test now says so. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit e4905b8 |
…e formatter **The clause and the hint could describe different windows** -- the very thing the first commit claimed was impossible. Splitting them into two helpers meant two `formatTimePickerDate` calls, and for a relative range (`now-15m`, the picker default) `now` resolves to a different millisecond on each. The hint's lower bound, parsed second, was always the later of the two, so its window was strictly narrower and an engine pruning on it could drop rows the clause matched. `getTimeFilter` now returns the clause and the bounds from a single parse; a test hammers a relative range 200 times asserting the clause carries exactly the reported bounds. **The SQL action must not receive the hint.** `SQLQueryRequest` allowlists its body fields (`SUPPORTED_FIELDS`) and, on anything else, `isSupported()` turns false and the query is silently routed to the legacy V1 engine -- a different dialect and response shape, with no error. Only PPL is genuinely lenient, so the endpoint allowlist is down to PPL alone and its comment now says why, per endpoint. **The DST fix had no teeth, so remove the hazard instead.** The jest preset pins TZ=UTC, where re-parsing a UTC bound as local time is an identity -- reintroducing the old `formatDate` wrap kept every test green, including the DST case added for it. Setting `process.env.TZ` in the test file does not help; the timezone is fixed before modules load. So `formatDate` is deleted outright, along with its tests, after fixing its last caller (`sql_search_interceptor`, which had the same bug and still shifted SQL time filters by an hour inside a spring-forward gap). A formatter that cannot be called cannot be reintroduced by accident. Smaller items from the same review: the `time_range` route schema now allows unknown keys, matching every other object in that schema, so a newer client is not rejected by an older server; the bound shape check accepts years past 9999 rather than silently dropping the hint; a non-null assertion is gone (the repo forbids them); and two gaps are covered -- a dataset that defers time filtering to the search strategy (`hideDatePicker === false`), and the SQL action not receiving the hint. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit bb59a0d |
✅ All unit and integration tests passing
|
The hint is inert unless the cluster sets plugins.query.pruning.enabled, so this switch can only subtract. It is still worth having: acting on the hint prunes indices before the schema is resolved, which narrows the merged mapping, so a field that only the pruned-away indices map stops resolving. That is the point when it removes a mapping conflict and a regression when a saved panel depended on the field -- and the breakage surfaces in Dashboards, while the cluster setting is the wrong lever for it, since turning that off also drops the shard and PIT savings for clients that do not go through Dashboards. Follows explore:enablePartialResults, which gates its own per-request field the same way. WORKSPACE-scoped when the workspace feature is on, like its sibling in this file, so one workspace can opt out on its own. Defaults to on, unlike partial results: sending the hint cannot change a response by itself, and the cluster setting it depends on is already off by default, so defaulting this off would mean two switches to find before anything happens. Reads fail-open when uiSettings has not resolved yet -- the hint is inert without the cluster setting, so an unresolved read cannot cause a surprise. Turning it off leaves the appended where clause untouched, which the new cases assert: the setting must change which indices are read, never which rows come back. Signed-off-by: Kai Huang <ahkcs@amazon.com>
opensearch-project/sql#5759 enables plugins.query.pruning.enabled by default, so 'has no effect unless the cluster also sets it' stops being true and reads as though the switch does nothing. Say what each side controls instead: the cluster decides whether to act on the bounds, this decides whether to send them, and either opting out is enough. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Align with the design the engine implements (opensearch-project/sql#5698, Approach 3): three flat parameters, time_field / start_time / end_time, rather than a nested time_range object of this plugin's own invention. The engine also accepts date math and ISO-8601 there, but we keep sending the clause's own literals -- a relative range resolves to a different instant on every parse, so anything else would let the bounds and the clause describe different windows. time_field carries the dataset's configured field rather than leaving the engine to assume @timestamp, which is what makes this reach most datasets at all: the sample-data pattern is configured on `timestamp`, and a mismatch there prunes nothing. Renames the advanced setting to query:enhancements:timeBounds to match, while it is still unreleased and the key can move for free. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit a3f7caa |
PR Code Suggestions ✨Latest suggestions up to b1e6c78 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 0885c35
Suggestions up to commit 4dc0d4f
Suggestions up to commit a3f7caa
|
The engine defaults an absent time_field to @timestamp. Forwarding start_time and end_time without it therefore does not merely lose the optimization -- it prunes on a field the dataset is not configured on, which is a wrong answer. The interceptor only builds bounds when the dataset has a time field, so all three are always available together; requiring that here removes the possibility rather than relying on it. Also renames TIME_RANGE_ENDPOINTS to TIME_BOUNDS_ENDPOINTS, since the engine's parameters are no longer called time_range. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 4dc0d4f |
|
Persistent review updated to latest commit 0885c35 |
`appendDefaultSort` (opensearch-project#12546) adds `| sort - <dataset time field>` unless the query already contains sort, stats, head, rare, top or rename. `chart` and `timechart` are absent, so the sort is appended after them -- and both replace the row type with a time column of their own choosing. `timechart` always names it `@timestamp`; `chart` names it whatever its `over` argument was. Neither is necessarily the field the dataset is configured on, and the client cannot tell. When they differ the appended clause cannot bind and the query fails outright: source = logs-* | chart count() over @timestamp by host -> ... | chart count() over @timestamp by host | sort - `timestamp` -> Field [timestamp] not found. That reaches any dataset whose time field is not what the charting command emits -- including OpenSearch Dashboards' own sample logs data, configured on `timestamp`, for which `timechart` can never bind since it hardcodes `@timestamp`. The histogram query this plugin builds alongside the results query fails the same way, so a user sees the error twice. Listed for the same defensive reason `rename` already is: it too keeps the time field, unless it happens to rename it away. Losing the default sort here costs nothing, since an aggregated result carries its own ordering, which is why stats, top and rare are excluded too. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit b1e6c78 |
| mockIsPPLSearchQuery.mockReturnValue(true); | ||
| mockPPLFilterUtils.convertFiltersToWhereClause.mockReturnValue(''); | ||
| mockPPLFilterUtils.getTimeFilterWhereClause.mockReturnValue(''); | ||
| mockPPLFilterUtils.getTimeFilter.mockReturnValue(''); |
There was a problem hiding this comment.
This mock returns a string, but the production code now destructures { clause, bounds } from it. That leaves clause undefined, and the tests in this block only pass because insertWhereCommand happens to tolerate it. Changing this to mockReturnValue({ clause: '' }) would keep the fixture consistent with the new signature.
| }); | ||
|
|
||
| it('forwards the time range so the engine can prune indices that cannot match it', async () => { | ||
| // Endpoint matters: the bounds only ride along on the PPL action (see TIME_RANGE_ENDPOINTS). |
There was a problem hiding this comment.
Small nit: the constant this comment points at is actually named TIME_BOUNDS_ENDPOINTS.
| 'top', | ||
| 'rename', | ||
| 'chart', | ||
| 'timechart', |
There was a problem hiding this comment.
Flagging this for other reviewers. Unlike the time-bounds fields, this change and the formatDate removal in the SQL interceptor take effect immediately instead of staying inert until an engine opts in. The description covers both clearly and they look correct to me. I just want to call out that these two ship real behavior changes, in case anyone reads the "no behaviour change ships with this PR" framing as covering the whole diff.
| partial_result: schema.maybe(schema.boolean()), | ||
| // Bounds of the time filter already present in the query text, forwarded so the | ||
| // engine can skip indices that cannot hold data in the range. | ||
| time_field: schema.maybe(schema.string()), |
There was a problem hiding this comment.
An observation rather than a blocker. The query:enhancements:timeBounds setting is enforced only in the client interceptor. This route and facet.ts forward the fields whenever they are present, so a stale or non-conforming client can still send bounds while the setting is off. Since the authoritative gate is the cluster's plugins.query.pruning.enabled, that seems acceptable. It may still be worth a sentence in the setting description clarifying that it governs sending, not acceptance.
2 participants
Description
The date picker appends a time filter to the PPL query text. The engine only sees that filter after it has resolved the schema of the queried index pattern — it merges the mappings of every index the wildcard matches before it parses the appended
where. So it cannot use the filter to skip indices that hold no data in the picked range, and a pattern spanning months of rollovers pays for all of them (their shards, their PIT contexts, their mapping conflicts) to answer a question about the last 30 minutes.This sends the same bounds a second time, out of band, so the engine has them before it resolves anything:
{ "query": { "query": "source=logs-* | ... | where `@timestamp` >= '2026-09-09 22:00:00.000' AND `@timestamp` <= '2026-09-09 22:30:00.000'", "language": "PPL", "time_field": "@timestamp", "start_time": "2026-09-09 22:00:00.000", "end_time": "2026-09-09 22:30:00.000" } }The parameter names and shape are the engine's, from Approach 3 of the index-pruning design — not this plugin's own invention.
Three properties worth calling out:
whereclause is unchanged and still does the filtering, so results are identical whether or not the engine reads these. Engines that don't read them ignore them — the PPL API parses its request body field by field, verified against a cluster below.FilterUtils.getTimeFilterreturns both from a single parse of the range. That is load-bearing rather than tidy:now-15mresolves to a different millisecond on every parse, so deriving the two separately made the hint's window strictly narrower than the clause's, and an engine pruning on it could skip an index the filter would have matched. A test hammers a relative range 200 times asserting the clause carries exactly the reported bounds. The engine also accepts date math here, but sending the clause's own literals is what makes the two agree by construction.time_fieldcomes from the dataset, not a hardcoded@timestamp, since the picker uses the index pattern's configured time field (timestampon the sample logs data, for instance). Without it the engine would default to@timestampand prune nothing for those datasets.All three fields are sent together or not at all. The engine defaults an absent
time_fieldto@timestamp, so forwarding bounds without it would not merely lose the optimization — it would prune on a field the dataset is not configured on, which is a wrong answer. The interceptor only builds bounds when the dataset has a time field, so all three are always available together; requiring it infacet.tsremoves the possibility rather than relying on it.The hint is attached exactly where the clause is appended, so it is absent whenever no clause is:
skipTimeFilter, a dataset with no time field, a non-search command such asdescribe, a dataset whosehideDatePickeroverride defers filtering to the search strategy, or a picked range that does not parse. Where the clause prefers a caller-supplied range over the global picker, the hint follows it.An advanced setting,
query:enhancements:timeBounds(default on), stops sending the bounds. Acting on them narrows the merged mapping, so a field that only the skipped indices map stops resolving — the point when it removes a conflict, a regression when a saved panel depended on that field. That breakage surfaces here, while the cluster setting is the wrong lever for it: turningplugins.query.pruning.enabledoff also drops the shard and PIT savings for clients that never go through Dashboards. Modelled onexplore:enablePartialResults, which gates its own per-request field the same way.Files touched, following
partial_resultas the precedent end to end:data/common/query/types.tsQuery.time_field/start_time/end_time, and theTimeBoundsinterfacequery_enhancements/public/search/filters/filter_utils.tsgetTimeFilterreturning clause + bounds from one parse;getTimeFilterWhereClause/getTimeFilterBoundsdelegate to itquery_enhancements/public/search/ppl_search_interceptor.tsbuildQueryattaches the bounds where it appends the time filter, subject to the settingquery_enhancements/server/ui_settings.ts,server/plugin.tsquery_enhancements/server/routes/index.tsquery_enhancements/server/utils/facet.tsquery_enhancements/public/search/sql_search_interceptor.ts,common/utils.tsformatDateand its last caller (see below)query_enhancements/public/search/ppl_search_interceptor.tschart/timechartadded toDEFAULT_SORT_BLOCKING_COMMANDS(see below)Issues Resolved
Front-end half of the index-pruning work for PPL. The engine-side consumer is opensearch-project/sql#5766, which implements Approach 3 of opensearch-project/sql#5698 and reads exactly these parameters. It is gated by
plugins.query.pruning.enabled, which opensearch-project/sql#5759 turns on by default.No behaviour change ships with this PR on its own; it is inert until an engine reads the fields — so it can merge in either order relative to the engine side. The engine change adds only request-body parameters, no new endpoint and no transport change, so an older cluster ignores them.
The one exception is the charting-sort fix below, which takes effect immediately and independently.
Screenshot
Payload of
POST /api/enhancements/search/pplfrom Explore with the picker on a 7-day window, showing the bounds beside the appended clause (this is the histogram query, which derives its interval from the global picker):The bounds match the clause literals exactly, and the field is the dataset's configured
timestamp.Testing the changes
Ran a dev server against a local 3.9 cluster with the SQL plugin (security enabled) on eight monthly indices mimicking a rollover that renamed a field's shape —
attributes.clusterwas an object holdingnamein the seven older ones and is a plain keyword in the newest — then queriedsource = prune-demo-2026.* | chart count() over timestamp by attributes.clusterfrom Explore.With the engine-side consumer in place, both queries Explore issues (the results query with its appended
sort, and the histogram with its appendedspan):plugins.query.pruning.enabledonCannot chart by [attributes.cluster] because it is an object.stats count()The third row is the one that matters for trusting it: the row count is unchanged. And the engine logs the substitution it made —
Pruned index expression from prune-demo-2026.* to prune-demo-2026.09, eight indices down to one.Probes through the OSD route:
describe <index>with bounds presenttimestampstart_timewithoutend_timeBackend tolerance checked directly against the cluster, bypassing OSD:
POST /_plugins/_pplwith these fields returns 200 and the samedatarowsas the identical query without them, so a cluster with no support for them ignores them.Unit tests added: the clause embeds exactly the reported bounds, parameterized over both engine flavors (bare literals and
TIMESTAMP('...')); a bound inside a DST spring-forward gap survives unshifted; an unparseable range reports no bounds rather thanInvalid datetext; the bounds are carried for a picked range, follow a caller-supplied one, and are absent with no time filter, no time field, or an unparseable range; the setting suppresses them while leaving the clause untouched (off / on / unset); and the server forwards them for the PPL action while omitting them for the SQL and async direct-query endpoints.yarn test:jestonquery_enhancements+data/common/query: 689 passed / 53 suites. Type-check error count is identical to the pre-change baseline (457 both ways), so this introduces none.Two review passes turned up five defects, fixed in later commits and worth recording here:
formatDateshifted bounds by an hour inside a DST spring-forward gap (pre-existing on both the PPL and SQL time filters, sinceformatTimePickerDatealready emits the target format in UTC). The wrap is gone from both call sites, and the helper is deleted with its tests: the jest preset pinsTZ=UTC, where the round-trip is an identity, so no test could have caught a reintroduction — removing the function is the only durable guard.SQLQueryRequestallowlists body fields and silently falls back to the legacy V1 engine on anything unexpected. The endpoint allowlist is PPL-only.Facet's body builder is shared with it; only S3 datasets' lack of a time field was preventing it.Invalid date(orNaN-aN-aN…) as a bound. Bounds are shape-checked; the clause keeps its existing behaviour, so a broken range still fails visibly instead of quietly dropping the filter.Known limitations, deliberately not addressed here: the bounds carry no source, and the engine applies them to every source the query reads (its documented scope, matching Splunk's picker), while the clause this plugin appends constrains only the outer pipeline — so a subsearch over a different pattern is pruned by bounds it was not filtered by;
formatTimePickerDatestill reports failure as a formatted string rather than a value a caller can test, which its six other callers also paper over; andTIME_BOUND_FORMATremains duplicated at several call sites that predate this change.Also fixed here: the default sort after a charting command
Not part of the time-bounds work, but found while testing it and cheap to carry rather than split into its own PR.
appendDefaultSort(#12546) adds| sort - <dataset time field>unless the query already containssort,stats,head,rare,toporrename.chartandtimechartare absent, and both replace the row type with a time column of their own choosing —timechartalways names it@timestamp,chartnames it whatever itsoverargument was. Neither is necessarily the field the dataset is configured on, and the client cannot tell:That reaches any dataset whose time field is not what the charting command emits — including this repo's own sample logs data, configured on
timestamp, for whichtimechartcan never bind since it hardcodes@timestamp. The histogram query built alongside the results query fails the same way, so the user sees the error twice.Listed for the same defensive reason
renamealready is: it too keeps the field, unless it happens to rename it away. Losing the default sort here costs nothing, since an aggregated result carries its own ordering — which is whystats,topandrareare excluded too. Verified against a live cluster: the query above returns 400 with the appended sort and 200 without.Check List
yarn test:jest—query_enhancements+data/common/query: 689 passed / 53 suitesyarn test:jest_integration