fix: push down date filter wrapped by a redundant date cast (v2 + Calcite) - #5681
fix: push down date filter wrapped by a redundant date cast (v2 + Calcite)#5681burnthm wants to merge 6 commits into
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 60c39b2)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 60c39b2 Explore these optional code suggestions:
Previous suggestionsSuggestions 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>
1 participant
Description
A PPL/SQL range comparison on a
date-mapped field does not push down to a native OpenSearchrangequery when the field is wrapped in a date function —timestamp(<field>)orCAST(<field> AS TIMESTAMP). Instead the engine emits a per-document script that 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 on large indices; the serialized script also embeds a per-request timestamp, so each execution recompiles and can trip the script-compilation-rate breaker. This is the shape the Grafana OpenSearch data source generates for its dashboard time filter, so users hit it without writing it themselves.Wrapping a field that is already of that same date/time type is a no-op for a comparison, so this PR folds the wrap back to the underlying field reference and lets the predicate push down to a native
range.Both engines are fixed, because the push-down decision lives in a different place on each and the v2 fix alone has no effect on 3.x (which uses Calcite by default):
LuceneQuerycanSupport()accepts a redundant date cast on the left operand;build()unwraps itPredicateAnalyzervisitCall()folds the wrap to the underlyingRexInputRefThe fold only applies when the cast is genuinely a no-op. Two guards, both exercised by negative tests:
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 isn't 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 in every case: only the execution plan changes, from
scripttorange.Validation
v2 and v3 return identical doc counts. 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: the fold produces arange; a type-changing cast (date()/time()over a timestamp field) and a cast over a non-date field both stay on the script path.PredicateAnalyzerTest: the fold produces aRangeQueryBuilder;date()over a timestamp field andlast_day()both stay on the script path.--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.