Drive PPL search command emission from the field's index mapping (#5682)

[penghuo]

Description

Two related fixes to how the search command builds its query_string filter, both on the Calcite path. 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.

1. Wildcards were dropped on values containing whitespace (#5682)

search source=idx name="foo bar*" against a keyword field returned 0 hits instead of matching the whole-value pattern. The parser marked any whitespace-containing literal as a phrase, emitting name:"foo bar*" — and inside a Lucene phrase * is a literal character, so the query looked for docs containing * in the stored value.

2. On text fields, a quoted value the analyzer splits became an OR over its tokens

The deeper cause was that SearchLiteral.isPhrase was set at parse time as value.contains(" ") — a syntactic test standing in for a semantic question: will the field's analyzer split this value? Whitespace is a poor proxy. foo=bar holds none, yet the standard analyzer splits it into [foo, bar].

So 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" matched any document holding just foo or just bar.

Emission strategy

PPL emits a single query_string filter; the Lucene query type is chosen by the parser at execution time, based on the quoted flag and how many tokens the field's analyzer produces. The emitter's job is to produce the right string, so it is now driven by the enclosing field's index mapping, read from AbstractOpenSearchTable.getFieldTypes().

The Calcite RelDataType round trip cannot supply this: OpenSearchTypeFactory collapses text to plain VARCHAR, erasing the text/keyword distinction. A TODO marks moving this metadata onto a RelDataType/scan annotation once the Calcite rule pipeline has been audited.

field mapping?
│
├── text | match_only_text
│     └── quoted AND NOT (wildcard without whitespace)?
│           ├── yes → quoted phrase
│           └── no  → unquoted, specials escaped, * and ? preserved
│
├── keyword | constant_keyword              (quoting irrelevant)
│     └── unescaped wildcard?
│           ├── yes → unquoted, escape specials INCLUDING whitespace
│           └── no  → quoted phrase
│
└── date | numeric | ip | boolean | unresolved
      └── legacy, untouched: contains a space ? quoted phrase : unquoted

text — honor the user's quoting. Unquoted passes through so wildcards stay operators; quoted becomes a phrase. The one exception is a whitespace-free value carrying a wildcard, which stays unquoted: quoting it would let the analyzer discard the wildcard, so body="foo*" would stop matching foobar. That exception is gated on whitespace because with a space an unquoted value splits into separate clauses and the tail loses its field binding.

keyword family — quoting carries no information: the analyzer is a no-op, so a quoted phrase and a bare term both resolve to the same single term (Lucene returns early from createFieldQuery when numTokens == 1, before quoted is read). Whole-value semantics are emitted instead. Escaping the whitespace in the wildcard form is fix #1.

other mappings — these do their own value parsing and are out of scope; left on the previous code path.

Measured contract

Hit counts from CalciteSearchCommandIT against two indices with identical documents — name mapped keyword in 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.

Row	PPL query	Text	Keyword
1.1	name=foo	7	1
1.2	name="foo"	7	1
2.1	name="foo_bar"	1	1
2.2	name="foo.bar"	1	1
2.3	name="foo-bar"	4 ← was 7	1
2.4	name="foo/bar"	4 ← was 7	1
2.5	name="foo@bar"	4 ← was 7	1
2.6	name="foo bar"	4	1
3.1	name=foo*	11	10
3.2	name="foo*"	11	10
3.3	name="foo_*"	1	1
3.4	name="foo.*"	1	1
3.5	name="foo-*"	0	1
3.6	name="foo/*"	0	1
3.7	name="foo bar*"	4	2 ← was 0 (#5682)
4.1	name="*foo"	7	1
4.2	name="*bar"	7	7
4.3	name="*foo bar"	4	1
5.1	name="f*r"	3	7
5.2	name="foo*bar"	3	7
5.3	name="foo *baz"	0	1
5.4	name="*foo bar*"	4	2
6.1	name="foo?"	1	1
6.2	name="?oo"	7	1
6.3	name="f?o"	7	1
6.4	name="foo?bar"	2	6
6.5	name="foo b?r"	0	1

Only the three Group 2 rows change, plus the reported bug in 3.7. Every wildcard row is unchanged, and the keyword column is unchanged apart from 3.7 — non-wildcard keyword values are single-token either way, so moving them to the quoted form is behavior-neutral.

Group 7 runs against a dedicated fixture — foo=bar, foo bar, foo, bar, baz — and asserts exact rows rather than counts, so the regression is visible:

Query	Field	Result
name="foo=bar"	text	foo=bar, foo bar — the single-token foo and bar docs are absent (they matched before)
name="foo=bar"	keyword	foo=bar — exact whole value
name="foo*"	text	11 — wildcard survives quoting
name=foo-bar / name="foo-bar"	keyword	identical; quoting is irrelevant

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_wildcard defaults to false, so the pattern is matched against the token dictionary where no token contains =. Quoted, the analyzer discards the * and the residual token must match exactly. The indexed tokens for foo=bar are [foo, bar] — the original value is not stored anywhere the query can reach. Keyword fields handle this correctly (3 hits on the same data), since the value is matched whole.

Breaking change

Text fields only: a quoted value that the analyzer splits is now a phrase rather than an OR over its tokens. Queries relying on the wider behavior will return fewer rows. Three examples in docs/user/ppl/cmd/search.md documented the over-matching and have been updated — one had ="cart-service" matching a service named cart, which the fix correctly stops.

Related Issues

Resolves #5682

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

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.

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d9d6568
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add null check for peek result The code assumes context.relBuilder.peek() returns a RelNode with a table, but doesn't verify the peek result is non-null before calling getTable(). If the builder's stack is empty or contains a node without table metadata, this will throw a NullPointerException. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [312-320] java.util.Map<String, ExprType> typesByName = new java.util.HashMap<>(); RelNode scan = context.relBuilder.peek(); -RelOptTable relOptTable = scan.getTable(); -if (relOptTable != null) { - AbstractOpenSearchTable osTable = relOptTable.unwrap(AbstractOpenSearchTable.class); - if (osTable != null) { - typesByName.putAll(osTable.getFieldTypes()); +if (scan != null) { + RelOptTable relOptTable = scan.getTable(); + if (relOptTable != null) { + AbstractOpenSearchTable osTable = relOptTable.unwrap(AbstractOpenSearchTable.class); + if (osTable != null) { + typesByName.putAll(osTable.getFieldTypes()); + } } } Suggestion importance[1-10]: 7 __ Why: Valid defensive programming suggestion. While relBuilder.peek() typically returns a non-null RelNode in this context (after visiting the child relation), adding the null check prevents potential NullPointerException if the builder state is unexpected.	Medium
	Detect all whitespace types consistently The whitespace check uses contains(" ") which only detects ASCII space (U+0020). Other whitespace characters like tabs, newlines, or non-breaking spaces will bypass the phrase-emission logic, potentially causing incorrect query splitting when the analyzer treats them as token separators. core/src/main/java/org/opensearch/sql/ast/expression/SearchLiteral.java [73-80] if (isTextLike(indexType)) { ... - boolean wildcardTerm = hasUnescapedWildcard(str) && !str.contains(" "); + boolean hasWhitespace = str.chars().anyMatch(Character::isWhitespace); + boolean wildcardTerm = hasUnescapedWildcard(str) && !hasWhitespace; return userQuoted && !wildcardTerm ? quoted(str) : unquoted(str); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that contains(" ") only checks for ASCII space. Using Character::isWhitespace would handle tabs, newlines, and other whitespace characters more comprehensively, improving robustness for edge cases where users might include non-standard whitespace in search values.	Low
	Handle trailing backslash edge case The escape-sequence handling is incomplete. When a backslash appears at the end of the string (e.g., "foo"), the code skips incrementing i but doesn't validate that a character follows. This could cause the trailing backslash to be treated as escaping nothing, potentially misclassifying strings ending with * or ?. core/src/main/java/org/opensearch/sql/ast/expression/SearchLiteral.java [120-132] private static boolean hasUnescapedWildcard(String s) { for (int i = 0; i < s.length(); i++) { char c = s.charAt(i); - if (c == '\\' && i + 1 < s.length()) { - i++; - continue; + if (c == '\\') { + if (i + 1 < s.length()) { + i++; + continue; + } + // Trailing backslash with no following character + break; } if (c == '*' || c == '?') { return true; } } return false; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that a trailing backslash could be handled more explicitly. However, the current code already handles this case safely by checking i + 1 < s.length() before incrementing, so a trailing backslash simply won't match the condition and the loop continues normally. The improvement adds clarity but doesn't fix a bug.	Low

---

Previous suggestions

Suggestions up to commit 8ad2755

Category	Suggestion	Impact
Possible issue	Handle consecutive backslashes correctly The method does not handle consecutive backslashes correctly. A sequence like \* (escaped backslash followed by wildcard) will incorrectly skip the wildcard, treating it as escaped when it should be considered unescaped. Track whether the previous character was an unescaped backslash. core/src/main/java/org/opensearch/sql/ast/expression/SearchLiteral.java [91-103] private static boolean hasUnescapedWildcard(String s) { + boolean escaped = false; for (int i = 0; i < s.length(); i++) { char c = s.charAt(i); - if (c == '\\' && i + 1 < s.length()) { - i++; + if (c == '\\' && !escaped) { + escaped = true; continue; } - if (c == '*' || c == '?') { + if ((c == '*' || c == '?') && !escaped) { return true; } + escaped = false; } return false; } Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a legitimate edge case where consecutive backslashes (\\*) could be mishandled. The improved logic with the escaped flag properly tracks escape state across iterations, fixing a potential bug in wildcard detection.	Medium
General	Add fallback for expression resolution If getOriginalExpression() returns non-null but toQueryString() throws an exception or returns null/empty, the fallback to getQueryString() is never attempted. Add error handling to ensure the pre-computed query string is used when expression-based resolution fails. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [307-324] if (node.getOriginalExpression() != null) { ... - queryString = node.getOriginalExpression().toQueryString(typesByName::get); + try { + queryString = node.getOriginalExpression().toQueryString(typesByName::get); + } catch (Exception e) { + queryString = node.getQueryString(); + } } else { queryString = node.getQueryString(); } Suggestion importance[1-10]: 6 __ Why: Adding error handling to fall back to getQueryString() when toQueryString() fails is a reasonable defensive programming practice. However, the suggestion assumes exceptions might occur without evidence from the PR context, making it a moderate improvement rather than a critical fix.	Low
	Verify space escape ordering The space replacement logic may fail if escapeLuceneSpecialCharacters introduces backslashes before spaces. This could result in \ becoming \ (double-escaped). Verify that the escape function does not already escape spaces, or apply space replacement before escaping special characters. core/src/main/java/org/opensearch/sql/ast/expression/SearchLiteral.java [62-64] if (isPhrase && !isTextLike(indexType) && hasUnescapedWildcard(str)) { - return QueryStringUtils.escapeLuceneSpecialCharacters(str).replace(" ", "\\ "); + String escaped = QueryStringUtils.escapeLuceneSpecialCharacters(str); + return escaped.replace(" ", "\\ "); } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about escape ordering, but the improved_code is identical to the existing_code, making the practical impact minimal. The concern is worth verifying but doesn't constitute a critical fix.	Low

[penghuo]

@vamsimanohar Please help review.

[vamsimanohar]

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.

[penghuo]

@vamsimanohar current doc seems a bug. please help take a look.

[penghuo]

@vamsimanohar current doc seems a bug. please help take a look.

[penghuo]

@vamsimanohar current doc seems a bug. please help take a look.