feat(query): add script query support - #87
Conversation
|
Reviewed for inclusion in v1.0.0-rc.9. Holding it. Two independent lenses (correctness and security) both landed on do-not-ship, and the headline reason is not a security one — the feature currently doesn't match any documents. Every finding below was sent to an independent verifier told to refute it; these are the ones that survived. Blocker — a top-level
|
|
Thank you for the review — both blockers verified directly before fixing, not taken on faith:
Not addressed in this pass (agree these are real, lower severity, tracking as follow-ups): no compiled-AST cache, request-time limit not applied to Full suite green: 🤖 Generated with Claude Code |
|
Housekeeping, not a review comment. We've just added a Contributor License Agreement to the project (#93). Until now contributions here were covered only by Apache-2.0 §5's inbound=outbound clause, which gives no explicit patent grant — worth tightening for a project this size. Once #93 merges, a
That PR is the signature — it comes from your own account, so the commit history is the record. To be explicit about something: we did not add anyone to the signed list on their behalf, including you. That file asserts a person has signed, and that's not ours to assert for someone else — hence the ask rather than a quiet edit. Sorry for the extra step on work that's already in flight. Thanks for the contributions. |
A standalone `script` query — a boolean filter (no scoring), distinct
from `script_score` (a function_score scoring function) — was
completely unparseable ("unknown query type `script`"), which broke
any client relying on it, e.g. OpenSearch's UBI sample dashboards use
it to filter on a computed field via a Painless boolean predicate.
Adds QueryNode::Script (ast.rs), its parser (parser.rs), planner
wiring (planner.rs always doc-scans it, like Percolate — no index can
back an arbitrary Painless predicate), and evaluation in
doc_matches_query (index.rs). The script must evaluate to an actual
boolean to match: erroring or returning any other type fails closed,
matching real Elasticsearch's script-cast-exception-drops-doc
behavior in filter contexts, rather than truthy-coercing a non-boolean
result.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PRCbyt7Y2HDyhG1tbQTeL
Addresses maintainer review findings on the original script-query PR —
all three verified directly before fixing:
- The query never matched anything via a real search. The only routing
change was in planner.rs's (dead) ExecutionPlan; the predicate that
actually gates the doc scan, is_doc_scan_query (index.rs), never
listed QueryNode::Script, so both the memtable and segment search
paths skipped it entirely. Reproduced live: indexed a document,
searched for it with a matching `script` query, got zero hits.
Fixed with one arm added to is_doc_scan_query, backed by a new
end-to-end test that goes through a real POST /{index}/_search
instead of calling doc_matches_query directly (which is what let the
original PR's tests pass despite the query being broken).
- A short script could abort the process. `apply_binary`'s string
concatenation had no size limit, so a flat (non-nested, non-deep)
sequence of `s = s + s;` statements doubles a string exponentially
per statement — a shape that trips neither MAX_PARSE_DEPTH (bounds
nesting, not statement count) nor MAX_EVAL_DEPTH (bounds recursion
depth, not string size). Added MAX_PAINLESS_STRING_LEN (1 MiB) and a
check on every concatenation.
- The filter failed open on the common case, contradicting its own
doc comment. Accessing `doc['missing_field'].value` (or any
arithmetic/comparison on a value that can't coerce to a number, most
commonly a missing doc/params field) silently produced 0.0 instead
of erroring, so `doc['typo'].value > -1` matched every document
instead of none. Real Painless throws unboxing a null into a
primitive; the evaluator now does too, in apply_binary, unary minus,
and resolve_doc_member's `.value` accessor — matching this crate's
own stated contract that script errors should be treated as
no-match/no-op by callers, not silently coerced to a default.
Not addressed here (tracked as follow-ups, lower severity): no
compiled-AST cache (re-parses per document), request-time script
limit not applied to _delete_by_query/_update_by_query/_validate,
boost/_name silently dropped, and the `{"script": {"script": "<src>"}}`
string-shorthand form isn't accepted.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PRCbyt7Y2HDyhG1tbQTeL
…ay ES does
Reconciles the script-query follow-up review with the closure-guard work
that landed in between. Three things had to be decided rather than merged.
1. `doc['missing'].value` keeps erroring — decided on ES's behaviour, not
on which patch was newer.
The follow-up proposed making it resolve to `null` so that
`doc['x'].value != null` could be used as a missing-field guard. That is
not what Elasticsearch does. Since 7.0, reading `.value` off an empty
`ScriptDocValues` throws:
A document doesn't have a value for a field! Use doc[<field>].size()==0
to check if a document is missing a field!
(6.x returned a type default with a deprecation warning; 7.0 removed
that.) So `!= null` is not the ES idiom — the exception fires before the
comparison is ever reached. The idiom ES documents, and names in its own
exception text, is `doc['x'].size() == 0 ? <default> : doc['x'].value`.
Erroring is also the only option that keeps a typo'd field name from
matching every document, which was the original reason for the
fail-closed change: `null` is falsy, but `doc['typo'].value > -1` would
then have to be rejected by an operator rather than by the field read,
and every accessor that grows a null-tolerant path reopens the hole.
Both requirements are met at once, because `.size()`/`.length`/`.empty`
are defined on a missing field and the ternary evaluates only the branch
it takes. Tests cover the idiom on present, absent, empty-array and
explicit-null shapes, and cover a typo'd field matching nothing at the
query surface.
`==`/`!=` do become null-aware, which the follow-up was right about for
`params`: `params` really is a `Map<String, Object>` in Painless, an
unsupplied key really does read as null, and comparing it to null really
is an ordinary reference comparison. Relational operators and arithmetic
are untouched and still throw on a null operand, as they do in ES.
The error message now matches ES's wording so it names the supported
guard instead of leaving the caller to guess.
2. Compiled scripts are memoised, bounded, and per-thread.
`eval_painless` re-tokenized and re-parsed the source for every document.
A legal 64 KiB script (the `MAX_SCRIPT_LEN` ceiling) cost ~2 ms/doc in
parsing alone, so per-document cost scaled with script SIZE rather than
complexity. That also defeated the doc-scan's cooperative timeout, which
polls its deadline every N docs assuming per-doc work is microseconds.
The key is caller-supplied text, so an unbounded map would itself be a
memory-exhaustion vector: the cache is capped at 128 entries AND 512 KiB
of retained source, whichever binds first, with tests asserting both
bounds and the byte accounting they rest on.
The cache is thread-local rather than a shared static because a compiled
AST holds `Rc`s — closure and local-function bodies are `Rc`-shared, and
deliberately non-atomic since a closure literal is cloned per invocation.
The ceiling is therefore per-thread and the total is bounded by worker
count, which is sized from cores rather than from request rate. It also
keeps a lock off a path taken once per document.
Parse failures are cached too — a malformed script is likewise evaluated
once per document, so leaving errors uncached would leave the same
per-doc parse cost reachable through invalid input.
3. `eval_painless` still runs its body through `exec_body`, so the
`CallGuard` depth/count closure limits and the implicit-last-value return
semantics are unchanged. The follow-up's inline loop predated that
extraction and was not a deliberate restructure; only the parse step
needed to change. Added coverage that the shared AST leaks neither
locals nor the per-evaluation call budget from one document to the next.
With the parse removed, executing a script still dominates the per-doc
cost: a benign 64 KiB script measures 385 us/doc on a release build, so
the old 4 096-doc poll interval was a ~1.6 s uninterruptible stretch.
The interval is now 256, bringing that to ~98 ms, at a cost of one
extra clock read per 256 docs -- noise against a >=3 us/doc JSON parse.
This is an improvement, not a bound, and the comment now says so. A
deliberately adversarial script of legal size -- flat, no closure calls,
tripping none of the existing limits -- measures 1.27 s/doc, which is
~325 s per poll interval. There is no per-evaluation CPU budget, so
nothing bounds that today; the poll interval only decides how often a
document boundary is checked. That gap is real and is left open here
rather than papered over.
7c8a88a to
3eabca3
Compare
|
Rebased onto current Three commits: your two, authorship preserved, plus one reconciliation commit. The follow-up and this PR contradicted each other on Since ES 7.0, reading The follow-up's underlying concern was still valid: a guarded script has to be runnable. That is addressed without loosening The Also corrected before merge: the comment and commit message claimed executing a max-size script is "~0.2 ms/doc" and that the 256-doc poll keeps the worst case "in the tens of milliseconds". Measured on a release build it is 385 us/doc for a benign 64 KiB script (~98 ms/poll) and 1.27 s/doc for a deliberately adversarial one (~325 s/poll). The tighter poll interval is a real improvement but not a bound, because nothing imposes a per-evaluation CPU budget. Both now say that rather than implying the gap is closed. Verified after the rebase: Thanks for this one — the script-query support is a real gap closed, and the fail-closed instinct on missing fields was right. |
Summary
scriptquery — a boolean filter (no scoring), distinct fromscript_score(afunction_scorescoring function) — was completely unparseable (unknown query type \script``), breaking any client relying on it. OpenSearch's UBI sample dashboards use it to filter on a computed field via a Painless boolean predicate ("Top Searches Without Results" panel).QueryNode::Script(ast.rs), its parser (parser.rs), planner wiring (planner.rsalways doc-scans it, likePercolate— no index can back an arbitrary Painless predicate), and evaluation indoc_matches_query(index.rs).Test plan
cargo fmt --checkcargo clippy --workspace --all-targets -- -D warningscargo test -p xerj-query -p xerj-engine --lib(254 passed; the 2 failures insnapshot_path_security_testsare pre-existing onmain, unrelated to this change — reproduced on baremaintoo)🤖 Generated with Claude Code
https://claude.ai/code/session_014PRCbyt7Y2HDyhG1tbQTeL