Fix PPL search command ignoring wildcards and over-matching quoted values (#5682) - #5697
Fix PPL search command ignoring wildcards and over-matching quoted values (#5682)#5697penghuo wants to merge 3 commits into
Conversation
…pensearch-project#5682) On the Calcite path, `search source=idx name="foo bar*"` against a keyword field returned 0 hits instead of matching the whole-value pattern `foo bar*`. The parser marked whitespace-containing literals as phrases, which emitted `name:"foo bar*"` — inside a Lucene phrase, `*` is a literal character, so it looked for docs containing `*` in the stored value and found none. Route emission per field mapping in SearchLiteral.toQueryString(ExprType): - text-like (text, match_only_text) → quoted phrase (unchanged) - non-text (keyword, etc.) with whitespace + unescaped wildcard → unquoted term with the space escaped, so query_string keeps the value as one whole-value pattern instead of splitting into two clauses - everything else (no whitespace, or phrase without wildcard) → legacy branches (unquoted-with-escapes, quoted phrase) The Calcite RelDataType round trip in CalciteRelNodeVisitor.visitSearch collapses `text` mapping to plain VARCHAR (OpenSearchTypeFactory:208), which erased the text/keyword distinction at the emitter. Read the ExprType map directly from AbstractOpenSearchTable.getFieldTypes() instead; TODO comment marks the follow-up to move this metadata onto a RelDataType/scan annotation once the Calcite rule pipeline is audited. Thread a `Function<String, ExprType>` resolver through the SearchExpression hierarchy (SearchComparison, SearchIn, SearchAnd/Or/Not/Group, SearchLiteral) so SearchLiteral can consult the resolved field's index type at emit time. Tests: 54 new Group1-Group6 tests in CalciteSearchCommandIT covering the full text × keyword × wildcard-placement matrix on a shared fixture, plus a core-level SearchLiteralTest for the emission decision table. Verified with `./gradlew doctest -DignorePrometheus` (85 tests) and `./gradlew -DignorePrometheus :integ-test:integTest` (30m36s, 0 failures). Signed-off-by: Peng Huo <penghuo@gmail.com>
PR Code Suggestions ✨Latest suggestions up to af99e7a Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit d9d6568
Suggestions up to commit 8ad2755
|
|
@vamsimanohar Please help review. |
vamsimanohar
left a comment
There was a problem hiding this comment.
Minor nit: java.util.Map and java.util.HashMap are used fully-qualified inline in visitSearch rather than imported at the top of the file. The rest of the file imports its types — consider moving these to the import block for consistency.
Otherwise LGTM — the decision tree is well-reasoned, test coverage is thorough, and the regression-safe fallback (unknown type → phrase form) is the right call.
Replaces the whitespace heuristic that decided phrase vs. term emission.
`SearchLiteral.isPhrase` was set at parse time as `value.contains(" ")`,
which is a syntactic test standing in for a semantic question: will the
field's analyzer split this value into multiple tokens? Whitespace is a
poor proxy — `foo=bar` and `foo-bar` hold none, yet the standard analyzer
splits both.
The consequence on a text field: the value was emitted unquoted,
query_string kept it as one field-scoped term, the analyzer split it, and
default_operator=OR combined the halves. `body="foo=bar"` therefore matched
any document holding just `foo` or just `bar`.
Emission is now selected by the enclosing field's mapping, read from
AbstractOpenSearchTable.getFieldTypes():
- text / match_only_text: honor the user's quoting. Unquoted passes
through so `*` and `?` stay query_string operators; quoted becomes a
phrase. Exception: a whitespace-free value carrying a wildcard stays
unquoted, because quoting would let the analyzer discard the wildcard
(`foo*` must keep matching `foobar`). That is only safe without
whitespace — with a space, unquoted would split into separate clauses
and the tail would lose its field binding.
- keyword / constant_keyword: quoting is irrelevant, since the analyzer is
a no-op and a quoted phrase resolves to the same single term as a bare
one. Emit whole-value semantics instead — a wildcard pattern when the
value holds an unescaped wildcard, otherwise an exact term. Whitespace
is escaped in the wildcard form so query_string keeps one clause.
- date / numeric / ip / boolean / unresolved: legacy behavior, untouched.
The v2 engine is unaffected. It reaches emission through the no-arg
SearchExpression.toQueryString(), which passes a null-returning resolver
and lands in the legacy branch.
Behavior change, text fields only: a quoted value the analyzer splits is
now a phrase rather than an OR over its tokens. On the test fixture,
`name="foo-bar"` / `"foo/bar"` / `"foo@bar"` go from 7 hits to 4. Wildcard
rows are unchanged. Three examples in docs/user/ppl/cmd/search.md
documented the old over-matching and have been updated.
Tests: Group 7 added to CalciteSearchCommandIT over a dedicated fixture
(foo=bar, foo bar, foo, bar, baz) so the single-token documents that used
to OR-match are asserted absent. SearchLiteralTest covers the three
mapping branches. Verified with CalciteSearchCommandIT, SearchCommandIT
(v2), :core:test, :ppl:test, doctest, and :integ-test:integTest.
Signed-off-by: Peng Huo <penghuo@gmail.com>
|
|
||
| ```ppl | ||
| search severityText="INFO" AND `resource.attributes.service.name`="cart-service" source=otellogs | ||
| search severityText="INFO" AND `resource.attributes.service.name`="cart*" source=otellogs |
There was a problem hiding this comment.
@vamsimanohar current doc seems a bug. please help take a look.
| |------------------------------| | ||
| | Microsoft.Extensions.Hosting | | ||
| +------------------------------+ | ||
| fetched rows / total rows = 2/2 |
There was a problem hiding this comment.
@vamsimanohar current doc seems a bug. please help take a look.
| | instrumentationScope.name | | ||
| |-----------------------------------------------------------------------------| | ||
| | Microsoft.Extensions.Hosting | | ||
| | go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc | |
There was a problem hiding this comment.
@vamsimanohar current doc seems a bug. please help take a look.
The previous commit rewrote non-wildcard keyword values from `field:value` to `field:"value"`. Both forms resolve to the same Lucene TermQuery — the keyword analyzer is a no-op, so Lucene returns from createFieldQuery at `numTokens == 1` before the quoted flag is read — but roughly a dozen expected-plan fixtures compare the emitted query_string as a string, and they broke. CalcitePPLBig5IT.sort_keyword_can_match_shortcut was the first to fail in CI on `process.name=kernel`. The rewrite was cosmetic. It was there to state "quoting is irrelevant on keyword" in code, but it fixed nothing: `x=y` and `a>b` already match correctly unquoted in term position, so there was no escaping gap to close either. Narrow the keyword branch to the case that is actually broken — a value holding an unescaped wildcard, which must reach Lucene as a single term for the pattern to apply to the whole stored value. Everything else on keyword falls through to the legacy branch and is byte-identical to before. The point about quoting being irrelevant now lives in the javadoc, as the reason not to rewrite the emission rather than something enforced by rewriting it. Text-side behavior is unchanged: a quoted value the analyzer splits is still a phrase rather than an OR over its tokens. No expected-output fixture is modified. Verified with CalcitePPLBig5IT, CalciteExplainIT, CalciteSearchCommandIT, :core:test, :ppl:test and doctest. Signed-off-by: Peng Huo <penghuo@gmail.com>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Description
Two ways the
searchcommand could return the wrong documents, both on the Calcite path.1. Wildcards were silently ignored when the value contained a space (#5682)
On a
keywordfield this returned nothing, instead of matchingfoo barandfoo barbaz. Any value containing whitespace was treated as a phrase, so the emitted filter wasname:"foo bar*"— and inside a Lucene phrase*is an ordinary character, so the query went looking for documents with a literal asterisk in them.2. On text fields, a quoted value could match far too much
This matched any document containing just
foo, or justbar. Reported from a real log search, wherebody="isTombstone==false"came back with lines whose only relevant content was"endOfBatch":false.The cause is shared with the first bug. Whether to emit a phrase was decided by
SearchLiteral.isPhrase, set at parse time asvalue.contains(" ")— a test for whitespace standing in for the real question: will this field's analyzer split the value into multiple tokens? Whitespace is a poor proxy for that.foo=barhas none, yet the standard analyzer splits it into[foo, bar].So the value went out unquoted,
query_stringkept it as a single field-scoped term, the analyzer split it, anddefault_operator=ORjoined the halves.How emission is decided now
The parser keeps whether the user quoted the value, and emission is selected from the field's index mapping — the thing that actually determines whether a value gets analyzed. The mapping is read from
AbstractOpenSearchTable.getFieldTypes(); the CalciteRelDataTyperound trip can't supply it, becauseOpenSearchTypeFactorycollapsestextto plainVARCHARand erases the distinction. ATODOmarks moving this onto aRelDataType/scan annotation once the Calcite rule pipeline has been audited.keyword with a wildcard — the keyword analyzer is a no-op, so the value has to arrive at Lucene as one term for the pattern to apply to the whole stored value. Escaping the whitespace stops
query_stringsplitting at the space and dropping the field binding on the tail. This is fix #1.text — honor the user's quoting. Unquoted passes straight through, so
*and?keep working as operators. Quoted becomes a phrase, which is how a user asks for "this whole value, in order" against the analyzed tokens. This is fix #2.There's one exception on text: a value with a wildcard and no whitespace stays unquoted. Quoting it would let the analyzer throw the wildcard away, so
body="foo*"would stop matchingfoobar. The exception is gated on whitespace because with a space, an unquoted value splits into separate clauses and the tail loses its field binding.everything else — left exactly as it was. Note that quoting genuinely carries no information on a keyword field: with a no-op analyzer, a quoted phrase and a bare term resolve to the same single term, because Lucene returns from
createFieldQueryatnumTokens == 1before it reads thequotedflag. That's a reason not to rewrite the emission — an earlier revision of this PR did, and broke ~12 expected-plan fixtures for no functional gain.The v2 engine is unaffected. It reaches emission through the no-arg
SearchExpression.toQueryString(), which passes a null-returning resolver and lands in the legacy branch.Measured behavior
Hit counts from
CalciteSearchCommandIT, run against two indices holding identical documents —namemappedkeywordin one,text(standard analyzer) in the other. 11 documents:foo,foobar,food,FOO,foo bar,foo barbaz,foo-bar,foo_bar,foo.bar,foo/bar,foo@bar.name=fooname="foo"name="foo_bar"name="foo.bar"name="foo-bar"name="foo/bar"name="foo@bar"name="foo bar"name=foo*name="foo*"name="foo_*"name="foo.*"name="foo-*"name="foo/*"name="foo bar*"name="*foo"name="*bar"name="*foo bar"name="f*r"name="foo*bar"name="foo *baz"name="*foo bar*"name="foo?"name="?oo"name="f?o"name="foo?bar"name="foo b?r"Four cells move: the three Group 2 text rows stop over-matching, and 3.7 keyword is the reported bug. Every wildcard row on text is unchanged, and the keyword column is unchanged apart from 3.7.
A separate fixture —
foo=bar,foo bar,foo,bar,baz— covers fix #2 with exact-row assertions rather than counts, so the regression is visible:name="foo=bar"foo=bar,foo bar— the single-tokenfooandbardocuments are absent; they matched beforename="foo=bar"foo=bar— exact whole valuename="foo*"name=foo-bar/name="foo-bar"Known limitation
On a text field, a wildcard combined with a character the analyzer splits on cannot match under any emission —
body="foo=ba*"returns 0.Unquoted,
analyze_wildcarddefaults tofalse, so the pattern is matched against the token dictionary, where no token contains=. Quoted, the analyzer discards the*and the residual token has to match exactly. The indexed tokens forfoo=barare[foo, bar]— the original value isn't stored anywhere the query can reach. Keyword fields handle this correctly (3 hits on the same data), because the value is matched whole.Breaking change
Text fields only. A quoted value that the analyzer splits is now a phrase instead of an OR over its tokens, so queries relying on the wider behavior will return fewer rows.
Three examples in
docs/user/ppl/cmd/search.mddocumented the over-matching and have been updated — one had="cart-service"matching a service actually namedcart, which the fix correctly stops.No expected-plan fixture is modified.
Related Issues
Resolves #5682
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.