fix: push down date range filters wrapped by redundant date conversions (both operands, v2 + Calcite) - #5681
Conversation
A PPL/SQL range comparison on a `date`-mapped field falls back to a per-document script when the field is wrapped in timestamp()/ CAST(... AS TIMESTAMP) — the shape the Grafana OpenSearch data source generates for its dashboard time filter — because LuceneQuery.canSupport() only accepts a bare reference on the left operand. The scripted path parses the timestamp per document (no BKD/points acceleration), which saturates the search thread pool on large indices. Wrapping an already date/time-typed field in a date/time cast is order-preserving (a no-op for a range comparison), so fold the redundant cast to the underlying field reference and let the predicate push down to a native range query. Restricted to OpenSearchDateType references so a genuine string/number->timestamp conversion still uses the script path. Resolves opensearch-project#5680 Signed-off-by: Tom Burns <burnthm@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit 04be095)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 04be095 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 9f4c7cb
Suggestions up to commit 60c39b2
Suggestions up to commit 24d4949
Suggestions up to commit 64dedec
Suggestions up to commit 8422367
|
Address automated review feedback on opensearch-project#5681: - unwrapReference now validates the operand via referenceWrappedByRedundantDateCast and throws IllegalStateException instead of performing an unchecked cast, making the canSupport()-before-build() precondition explicit rather than risking a ClassCastException. - referenceWrappedByRedundantDateCast stores the inner argument in a local to avoid evaluating getArguments().get(0) twice. No behavior change; existing tests unaffected. Signed-off-by: Tom Burns <burnthm@amazon.com>
|
Persistent review updated to latest commit 64dedec |
|
On v3, a comparison whose field side is a CAST(... AS TIMESTAMP) / timestamp(field) (SqlKind.CAST, handled by supportedRexCall/visitCall → toCastExpression) won't fold to a bare field reference, so it falls back to a ScriptQueryBuilder instead of emitting a native rangeQuery, which is the same per-document parsing cost this issue describes, just on the Calcite engine. Users on the v3 engine (or with plugins.calcite.enabled) would still hit the regression. Proposed follow-up: in PredicateAnalyzer, when the operand of a comparison is a CAST/date-builtin over a field whose OpenSearch type is already an OpenSearchDateType, unwrap it to the underlying field ref before building the range (mirroring referenceWrappedByRedundantDateCast/unwrapReference here), with the same guardrail: only unwrap when the inner field is genuinely date-typed so non-order-preserving conversions still use the script path. Can handle this as a separate follow-up PR to keep this one focused on the v2 path, or fold it into this PR if reviewers prefer a single change. |
The redundancy check accepted any date/time cast over any date/time-typed
field without requiring the two to match, so a cast that changes the
date/time type was folded away and silently changed results:
date(<timestamp field>) truncates the time component, so
date(ts) <= '2024-01-15' is not ts <= '2024-01-15'
Against docs at 2024-01-15 08:00 and 2024-01-15 23:00 the predicate
correctly matches 2 rows; folded to the bare field it matched 0.
time(<timestamp field>) extracts the time of day, which is not even
monotonic with respect to the timestamp (23:00 on day 1 sorts after
01:00 on day 2), so no range rewrite is valid.
Map each cast function to the type it produces and fold only when that
target equals the field's own type, i.e. when the cast is genuinely a
no-op. This keeps the intended case -- timestamp()/CAST(... AS TIMESTAMP)
over a `date`-mapped field -- pushing down to a native range query.
Also addresses automated review feedback: add explicit parentheses to
canSupport() so the intended grouping of the size/operand checks against
the isMultiParameterQuery alternative is unambiguous.
Signed-off-by: Tom Burns <burnthm@amazon.com>
Calcite/v3 counterpart of the v2 LuceneQuery fold. PredicateAnalyzer only pushes a comparison down to a native range query when the operand is a bare field reference, so wrapping a date field in timestamp()/CAST(... AS TIMESTAMP) makes the whole predicate fall back to a per-document script that parses the timestamp for every scanned document. Without this the v2 fold has no effect on the Calcite engine, which 3.x uses by default. Fold the wrap to the underlying field reference when it is a genuine no-op. Because date/time values are modelled as UDTs whose backing SqlTypeName is VARCHAR, the UDT (EXPR_TIMESTAMP/EXPR_DATE/EXPR_TIME) identifies the cast target rather than getSqlTypeName() -- which always reports VARCHAR and so never matches a date/time type. As in the v2 path, the fold requires the cast target to match the field's own type exactly. date(<timestamp field>) truncates the time component and time(<timestamp field>) extracts the time of day (not monotonic in the timestamp), so those remain on the script path. Validation -- v2 and v3 return identical doc counts --------------------------------------------------- Both engines were run against an identical 100k-doc dataset (fixed RNG seed, same insertion order; `event_action` histogram verified equal on both nodes: break_enter=9935, cue_message=1965, gar=1946, impression=76143, other=10011). Nodes: 2.19.0 + v2 fold, and 3.7.0 + this Calcite fold. Predicate window 2026-08-04 17:42:40..20:42:40, selective filter event_action in (gar, cue_message). query shape v2 (2.19) v3 (3.7) counts ------------------------------------------------------------------------ pure date, timestamp() wrap RANGE / 37507 RANGE / 37507 match pure date, CAST(..) wrap RANGE / 37507 RANGE / 37507 match pure date, bare field (control) RANGE / 37507 RANGE / 37507 match + selective, no head RANGE / 1488 RANGE / 1488 match + selective, head after where RANGE / 1488 RANGE / 1488 match + selective, head before where RANGE / 385 RANGE / 385 match Every shape plans a native range on both engines and returns the same count, including the bare-field control -- so the fold reproduces bare-field semantics exactly rather than merely agreeing with itself. Correctness of the type-match guard was checked separately on a 3-doc deterministic index (docs at 2024-01-15 08:00, 2024-01-15 23:00, 2024-01-16 01:00), comparing stock 3.7 against the patched build: timestamp(ts)/CAST(ts AS TIMESTAMP) flip SCRIPT -> RANGE with unchanged counts, while date(ts) and time(ts) stay on the script path with unchanged counts. Unguarded, date(ts) <= '2024-01-15' would have folded to ts <= '2024-01-15' and returned 0 rows instead of 2. Signed-off-by: Tom Burns <burnthm@amazon.com>
|
Persistent review updated to latest commit 24d4949 |
v3 (Calcite) fix added, plus a correctness fix to the v2 pathFollowing up on my earlier note about the Calcite engine — I've folded the v3 change into this PR rather than a separate one, since the two halves share the same rule and the same guard. Two new commits:
1. Latent correctness bug in the original v2 commit (please look here first)My first version checked "is this a date/time cast?" and "is the operand a date-typed field?" but never that the two matched. That folds away casts which are real conversions, silently changing results:
Fixed by mapping each cast function to the type it produces and folding only when that target equals the field's own type — i.e. only when the cast is genuinely a no-op. The intended case ( 2. The v3/Calcite half
One implementation gotcha worth flagging for reviewers: date/time values are modelled as UDTs whose backing 3. Validation — v2 and v3 return identical doc countsBoth engines were run against an identical 100k-doc dataset (fixed RNG seed and insertion order;
Every shape plans a native range on both engines and returns the same count — including the bare-field control, so the fold reproduces bare-field semantics rather than merely agreeing with itself. (Latency isn't compared across the two lines here; that isn't apples-to-apples across major versions. Same-engine before/after numbers are in #5680.) The type-match guard was checked separately on a 3-doc deterministic index, stock vs patched: 4. Automated review suggestions
Open question for maintainersShould the Calcite half ship here or as its own PR? I kept it together because the type-match guard needs to be identical on both paths, but I'm happy to split it if you'd rather review them separately. |
The Calcite check keyed off the call's result type, so any single-argument
function returning a date/time UDT over a field of that same type was folded
to the bare field. That is wrong for functions which change the value rather
than just reinterpret it.
LAST_DAY is the clearest case: it takes one date/time argument and returns
DATE, so over a DATE-typed field (e.g. a field mapped with format
`yyyy-MM-dd`) it was rewritten to a range on the raw field. Against docs
d=2024-01-15 and d=2024-01-31:
last_day(d) = '2024-01-31' correct: 2 rows (both fall in January, and
last_day maps both to 2024-01-31)
folded to d = '2024-01-31' returned 1 row
Require the call to be a CAST or one of the timestamp()/date()/time()
conversion operators before considering the type match, mirroring the
function whitelist already used on the v2 path. Verified on a 3.7.0 node:
last_day() now stays on the script path while timestamp()/CAST(... AS
TIMESTAMP) still fold to a native range.
Signed-off-by: Tom Burns <burnthm@amazon.com>
burnthm
changed the title
|
Persistent review updated to latest commit 60c39b2 |
The unit tests build the predicate tree by hand, so they cannot show that a real PPL query plans into the shape the fold matches, nor that the generated DSL is accepted by a cluster and returns the same rows. Both gaps mattered here: the Calcite check originally keyed off getSqlTypeName(), which always reports VARCHAR for a date UDT, so it was dead code that unit tests could not have caught. Add three tests, run against a real cluster: - ExplainIT.testFilterTimestampWrappedFieldPushDownExplain -- a timestamp()-wrapped filter on a timestamp field pushes down to a native range query. The generated request is byte-identical to the bare-field fixture (explain_filter_push_compare_timestamp_string), so the fold really does reproduce bare-field behaviour rather than merely something similar. - ExplainIT.testFilterLastDayOverDateFieldNoPushDownExplain -- last_day() over a DATE-typed field is not folded. This is the regression guard for the case where keying off the result type alone rewrote the predicate into a range on the raw field and returned the wrong rows. - CastFunctionIT.testRedundantDateCastOnFilteredFieldDoesNotChangeRows -- the wrapped and bare forms return identical rows, so "only the plan changes" is enforced rather than asserted. The explain tests inherit into CalciteExplainIT and CalciteNoPushdownIT, so each one covers all three engine configurations: v2, Calcite with pushdown, and Calcite with pushdown disabled. Expected plans added for all three. Signed-off-by: Tom Burns <burnthm@amazon.com>
dai-chen
left a comment
dai-chen
left a comment
There was a problem hiding this comment.
Thanks for the changes! A high level question: I checked https://github.com/grafana/opensearch-datasource/blob/main/pkg/opensearch/client/ppl_request.go but don't see it wraps timestamp field. Could you double check if any simplification or more changes required in this PR?
canSupport() accepted the value operand only as a literal or a
CAST_TO_*(literal). A bound written as timestamp('...') therefore fell to
the per-document script path even though the filtered field was a bare
reference:
where `event_time` >= timestamp('2026-08-04 17:42:40')
This is the shape clients emit for a time-range bound -- the Grafana
OpenSearch data source builds its PPL time filter exactly this way (see
pkg/opensearch/client/ppl_request.go, which formats
"where `%s` >= timestamp('%s')"). PPL coerces the string argument first, so
the operand arrives as timestamp(cast_to_timestamp('...')): the function
name is `timestamp`, which is not in castMap, and its argument is a cast
rather than a literal, so both halves of the existing check fail.
Resolve the value operand to the cast that should be evaluated plus the
literal to evaluate it on, accepting the timestamp()/date()/time()
builtins in addition to CAST_TO_*. Applying timestamp() to a value that is
already a timestamp is a no-op, so timestamp(cast_to_timestamp(literal))
evaluates the inner cast alone. The cast is resolved rather than evaluated
directly so castMap still parses the literal against the field's declared
date formats. A conversion that changes the date/time type of the inner
cast is not a no-op and is left on the script path.
Verified on a real cluster: `birthdate > timestamp('2016-12-08 00:00:00')`
now emits two native range queries on birthdate with no script clause,
matching the plan produced by the bare-field form.
Note the Calcite (v3) engine already pushed this shape down; the gap was
v2-only, which is the engine the reported regression was seen on.
Signed-off-by: Tom Burns <burnthm@amazon.com>
burnthm
changed the title
|
Persistent review updated to latest commit 9f4c7cb |
Yes, you're right @dai-chen. The Grafana code points to a different timestamp behavior and timeFilter := fmt.Sprintf(" where `%s` >= timestamp('%s') and `%s` <= timestamp('%s')", timeField, from, timeField, to)Especially since this was the original issue being raised 😅. Correction: Grafana wraps the bound, not the fieldI originally decoded the
i.e. a time-range filter on I re-decoded the serialized script from example slow logs running the sample query and walked the resulting decoded which matches the Why it still scripted — and what I'd missedWith a bare field on the left, the blocker is the value operand. I confirmed on a 2.19 node with only the original (field-side) fold applied:
So your question surfaced an important gap: the PR did not cover the shape that caused the reported impact. The reported symptom was unchanged (the CPU is burned by scripted What the PR now contains — two fixes, one per operand
Verified on a real cluster: On simplificationBoth folds are deliberately narrow, because two earlier iterations of the redundancy check were wrong and I'd rather the guards stay explicit:
|
Resolving a date conversion on the bound re-formats the literal into the
field's declared format, so doing it across date/time types changes which
documents match. `d >= timestamp('2024-01-15 12:00:00')` against a
DATE-typed (`yyyy-MM-dd`) field previously stayed on the script path and
matched 2 rows; resolving the conversion truncated the bound to
`2024-01-15` and matched 3.
Require the conversion to produce the type the field already has before
resolving it. The reported shape is unaffected (a `timestamp()` bound on a
`date`-mapped field, which is TIMESTAMP). The pre-existing
`CAST_TO_*(literal)` path is left exactly as it was.
Signed-off-by: Tom Burns <burnthm@amazon.com>
|
Persistent review updated to latest commit 04be095 |
|
A few followup notes on 04be095.
|
2 participants
Description
A PPL/SQL range comparison on a
date-mapped field can fall back to a per-document script instead of pushing down to a native OpenSearchrangequery, whenever either operand is wrapped in a redundant date/time conversion. The scripted path callscastToTimestamp→LocalDateTime.parsefor every scanned document; with no BKD/points acceleration this string-parses the timestamp per document across all shards and saturates the search thread pool. The serialized script also embeds a per-request timestamp, so each execution recompiles and can trip the script-compilation-rate breaker.canSupport()gates push-down on both operands, and each side has its own gap. This PR fixes both:1. Field operand — fold a redundant conversion on the filtered field. Push-down required the left operand to be a bare field reference, so
timestamp(<field>)orCAST(<field> AS TIMESTAMP)disqualified it. Wrapping a field that is already of that same date/time type is a no-op, so the wrap is folded back to the field reference.2. Value operand — resolve a bound wrapped in a date conversion. Push-down accepted the right operand only as a literal or a
CAST_TO_*(literal). A bound written astimestamp('...')therefore scripted even with a bare field on the left:This is the shape clients emit for a time-range bound. The Grafana OpenSearch data source builds its PPL time filter exactly this way —
ppl_request.goformatswhere `%s` >= timestamp('%s'), i.e. bare field, conversion around the literal. PPL coerces the string argument first, so the operand arrives astimestamp(cast_to_timestamp('...')): the function name istimestamp, which is not incastMap, and its argument is a cast rather than a literal, so both halves of the existing check fail. The value operand is now resolved to the cast to evaluate plus the literal to evaluate it on, accepting thetimestamp()/date()/time()builtins alongsideCAST_TO_*. The cast is resolved rather than evaluated directly socastMapstill parses the literal against the field's declared date formats.Fix 1 applies to both engines (
LuceneQueryfor v2,PredicateAnalyzerfor Calcite/v3) because the push-down decision lives in a different place on each and the v2 change alone has no effect on 3.x, which uses Calcite by default. Fix 2 is v2-only — Calcite already pushed that shape down.Both folds apply only where the conversion is genuinely a no-op. Three guards, each with a negative test:
date(<timestamp field>)truncates the time component (sodate(ts) <= '2024-01-15'is notts <= '2024-01-15'), andtime(<timestamp field>)extracts the time of day, which is not even monotonic in the timestamp.CASTand thetimestamp()/date()/time()conversion operators qualify. Result type alone is insufficient — other single-argument functions also return a date/time type while changing the value,LAST_DAYbeing the clearest example.Behaviour is unchanged throughout: only the execution plan changes, from
scripttorange.Validation
The value-operand fix, on a real cluster.
birthdate > timestamp('2016-12-08 00:00:00')(the client-generated shape) now emits two nativerangequeries onbirthdatewith no script clause — the same plan as the bare-field form. Pinned byExplainIT.testFilterTimestampWrappedBoundPushDownExplain.v2 and v3 return identical doc counts for the field-operand fold. Both engines run against an identical 100k-doc dataset (fixed RNG seed and insertion order;
event_actionhistogram verified equal on both nodes). Nodes:2.19.0+ v2 fold,3.7.0+ Calcite fold.timestamp()wrapCAST(..)wrapheadheadafterwhereheadbeforewhereEvery shape plans a native range on both engines and returns the same count, including the bare-field control — so the fold reproduces bare-field semantics rather than merely agreeing with itself.
Performance (single node, 100k docs, identical dataset, median of 5). On the unpatched node the bare-field form of the same filter runs in 25 ms while the wrapped form takes 628 ms — a ~25× penalty purely from the wrap. After the fix the wrapped form is back to bare-field parity:
timestamp()wrap)script628 msrange20 msCASTwrap)script550 msrange18 msrange25 msrange18 msheadscript53 msrange19 msheadafterwherescript122 msrange58 msheadbeforewherescript396 msrange218 msShapes with a selective filter look milder only because that filter pushes down natively and leads the Lucene conjunction, so the per-document script runs on a small matching subset (~1.5k of 100k).
Guard cases were verified on a 3-doc deterministic index:
timestamp(ts)/CAST(ts AS TIMESTAMP)flipscript→rangewith unchanged counts, whiledate(ts),time(ts)andlast_day(d)stay on the script path with unchanged counts.Related Issues
Resolves #5680
Check List
FilterQueryBuilderTest: both folds produce arange; a type-changing conversion on either operand, and a cast over a non-date field, stay on the script path.PredicateAnalyzerTest: the fold produces aRangeQueryBuilder;date()over a timestamp field andlast_day()stay on the script path.ExplainIT(inherited byCalciteExplainITandCalciteNoPushdownIT, so each covers v2, Calcite-with-pushdown and Calcite-without-pushdown): the wrapped-bound and wrapped-field shapes push down to a native range;last_day()does not.CastFunctionIT: the wrapped and bare forms return identical rows.--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.