-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ES|QL: support for parameters in LIKE and RLIKE #138051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ES|QL: support for parameters in LIKE and RLIKE #138051
Conversation
Use inline example since it can't be specified in csv-spec
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Pinging @elastic/kibana-esql (ES|QL-ui) |
fang-xing-esql
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @cimequinox ! The change in the grammar and parser looks good to me. I added some comment around testing positional and anonymous parameters(they should already work, just to make the tests cover more varieties of parameters), and combining parameter related errors in the ExpressionBuilder into one ParsingException and report them together at the end, the rest look pretty good to me.
| Expression left = expression(ctx.valueExpression()); | ||
| Literal patternLiteral = visitString(ctx.string()); | ||
| EsqlBaseParser.StringOrParameterContext right = ctx.stringOrParameter(); | ||
| String patternString = stringFromStringOrParameter(source, "LIKE", right); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use ctx.LIKE().getText() to get the operator name, same for the other three methods below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change. Done.
| } | ||
| if (pctx instanceof EsqlBaseParser.InputNamedOrPositionalParamContext inopctx) { | ||
| psrc = source(inopctx); | ||
| QueryParam param = paramByNameOrPosition(inopctx.NAMED_OR_POSITIONAL_PARAM()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a mechanism to collect all potential missing-parameter errors in paramByNameOrPosition. These errors are collected into the ParsingContext, and then combined and reported at the end of parsing, allowing users to see all parameter-related issues at once. The other two callers of paramByNameOrPosition can be used as references, they do not error out immediately when a parameter is missing. The same approach can be applied to the two ParsingExceptions later in this method.
testInvalidPositionalParams in StatementParserTests shows multiple query parameter related errors are combined and reported together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing this out.
I'll review the code you mention and revise the error handling.
I've revised the error handling to collect parameter errors instead of raising them immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes from code I've seen:
Within the ExpressionBuilder for parameter-related logic we try to accumulate parameter related exceptions with context.params().addParsingError() instead of immediately throwing them so that we may provide more complete error reporting if there are multiple problems with parameters.
For example in x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java
│ context.params()
▶▶│ .addParsingError(new ParsingException(source(node), "No parameter is defined ...
then later in x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
│ protected LogicalPlan plan(ParseTree ctx) {
│ LogicalPlan p = ParserUtils.typedParsing(this, ctx, LogicalPlan.class);
│ if (p instanceof Explain == false && p.anyMatch(logicalPlan -> logicalPlan inst ...
│ throw new ParsingException(source(ctx), "EXPLAIN does not support downstrea ...
│ }
│ if (p instanceof Explain explain && explain.query().anyMatch(logicalPlan -> log ...
│ // TODO this one is never reached because the Parser fails to understand mu ...
│ throw new ParsingException(source(ctx), "EXPLAIN cannot be used inside anot ...
│ }
▶▶│ var errors = this.context.params().parsingErrors();
│ if (errors.hasNext() == false) {
│ return p;
│ } else {
▶▶│ throw ParsingException.combineParsingExceptions(errors);
│ }
│ }
However we do throw ParsingException in various other situations.
With respect to this change, I think it is relatively easy to collect LIKE/RLIKE errors if we are also allow temporary use of "invalid" patterns or object for LIKE/RLIKE expressions. I've updated the logic to follow this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to combine all parameters related errors into one ParsingException and reported them together at the end. For a lot of other types of parsing errors we do throw them immediately.
| } | ||
| } | ||
|
|
||
| public void testParamsWithLike() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a few more tests that cover positional parameters (fields like ?1) and anonymous parameters (fields like ?)? We ran into issues with named parameters with array value (for example, {"pattern": ["a", "b"]}) previously, so it would be great to include some validation tests for that case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add some use of positional and anonymous parameters to the tests.
I've changed the StatementParserTests, the AnalyzerTests and the RestEsqlTestCase tests to make use of anonymous, positional and named parameters.
It did not occur to me to consider array values but they could be useful.
For example in addition to
field LIKE (?p1, ?p2) with params {"p1": "a*", "p2": "b*"}
we could allow
field LIKE ?parray with params {"parray": ["a*", "b*"]}
but I can imagine this could get complex if we allow both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did not occur to me to consider array values but they could be useful. For example in addition to
field LIKE (?p1, ?p2)with params{"p1": "a*", "p2": "b*"}we could allowfield LIKE ?parraywith params{"parray": ["a*", "b*"]}but I can imagine this could get complex if we allow both.
Yes, supporting array parameters could add extra complexity, we got a similar request to support array parameters for inlist predicate previously, and it is not supported yet, so it is not required for this PR, making sure clear error messages are returned is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR description suggesting we consider array parameter support in a followup change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the base_conversion index out of convenience, I ran a few queries passing arrays as parameters and noticed some curious behavior.
{
"query": "FROM base_conversion | WHERE string LIKE (?, ?) | KEEP string",
"params": ["f*", "0*"]
}
string
---------------
0xff
ff
0x32
but this array query returns no results without generating any error
{
"query": "FROM base_conversion | WHERE string LIKE ?patterns | KEEP string",
"params": [{"patterns": ["f*", "0*"]}]
}
string
---------------
looking at the plan for each, the first becomes
ExchangeSinkExec[[string{f}#116],false]
\_ProjectExec[[string{f}#116]]
\_FieldExtractExec[string{f}#116]<[],[]>
\_EsQueryExec[base_conversion], indexMode[standard], ...
"esql_single_value" : {
"field" : "string",
"next" : { ⎞ LIKE with two patterns
"expressionQueryBuilder" : { ⎥
"field" : "string", ⎥
"expression" : "string LIKE (?, ?)" ⎠
}
},
"source" : "string LIKE (?, ?)@1:30"
}
}], tags=[]}]]
but the second becomes
ExchangeSinkExec[[string{f}#120],false]
\_ProjectExec[[string{f}#120]]
\_FieldExtractExec[string{f}#120]<[],[]>
\_EsQueryExec[base_conversion], indexMode[standard], ...
"esql_single_value" : {
"field" : "string",
"next" : {
"term" : { ⎞ LIKE pattern ["f*","0*] turned into this
"string" : { ⎥
"value" : "[[66 2a], [30 2a]]", ⎥
"boost" : 0.0 ⎠
}
}
},
"source" : "string LIKE ?patterns@1:30"
}
}], tags=[]}]]
A simple WHERE string LIKE ?pattern with "params": [{"pattern" : "f*"}] becomes
ExchangeSinkExec[[string{f}#128],false]
\_ProjectExec[[string{f}#128]]
\_FieldExtractExec[string{f}#128]<[],[]>
\_EsQueryExec[base_conversion], indexMode[standard], ...
"esql_single_value" : {
"field" : "string",
"next" : {
"wildcard" : { ⎞ LIKE pattern "f*" was translated to a wildcard
"string" : { ⎥
"wildcard" : "f*", ⎥
"boost" : 0.0 ⎠
}
}
},
"source" : "string LIKE ?pattern@1:30"
}
}], tags=[]}]]
I think in the array case it slips by because the "type" of the array is the type of the first element so it gets passed farther than expected.
This PR needs a little more error checking to properly prohibit array valued parameters and generate a useful error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an invalidListParameter function to explicitly check for list parameters and a test to verify they are prohibited in LIKE/RLIKE expressions for now.
With this change the query which previously passed along the stringified list
{
"query": "FROM base_conversion | WHERE string LIKE ?patterns | KEEP string",
"params": [{"patterns": ["f*", "0*"]}]
}
now fails as expected.
{
"error": {
...
"type": "parsing_exception",
"reason": "line 1:30: Invalid pattern parameter type for LIKE [?patterns]: expected string, found list"
},
"status": 400
}
...rc/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/regex/RLike.java
Outdated
Show resolved
Hide resolved
.../java/org/elasticsearch/xpack/esql/expression/function/scalar/string/regex/WildcardLike.java
Outdated
Show resolved
Hide resolved
| if (pctx instanceof EsqlBaseParser.InputParamContext ipctx) { | ||
| psrc = source(ipctx); | ||
| QueryParam param = paramByToken(ipctx.PARAM()); | ||
| if (param != null && invalidListParameter(source, psrc, opname, param)) { | ||
| return invalid; | ||
| } | ||
| exp = visitInputParam(ipctx); | ||
| } | ||
| if (pctx instanceof EsqlBaseParser.InputNamedOrPositionalParamContext inopctx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we reuse the existing visitParam/visitInputNamedOrPositionalParam by just calling visit(pctx), instead of the 2 ifs?
I'm not sure if that's fully possible, but this function feels like reimplementing code that we have already in those other methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Originally I simply delegated to those and added the extra checking for list values later. I can probably simplify this with an additional argument to specify that in this path list values should be rejected.
I've refactored the code a bit to use a helper class which reuses visitParam. I hope the code is clearer.
…expression/function/scalar/string/regex/RLike.java ReST ─▶ REST Co-authored-by: Iván Cea Fontenla <ivancea96@outlook.com>
…expression/function/scalar/string/regex/WildcardLike.java ReST ─▶ REST Co-authored-by: Iván Cea Fontenla <ivancea96@outlook.com>
# Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java
Outdated
Show resolved
Hide resolved
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java
Show resolved
Hide resolved
julian-elastic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing my comments
fang-xing-esql
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @cimequinox!
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
?parameter support for LIKE / RLIKE
Background
The ES|QL LIKE and RLIKE operators currently support matching a field against a pattern string or list of pattern strings. For example
ESQL 131356 would like to extend this to support use of ? rest query placeholder parameters. E.g.
Implementation Plan
Add new CsvTests if the csv spec framework can simulate rest query parameters.Antlr Grammar
ESQL 131356 will make a minor change to x-pack/plugin/esql/src/main/antlr/parser/Expression.g4 which holds the parser production rules for the LIKE and RLIKE operators.
Supporting ? parameters in the parser can be accomplished with a simple extension
Generated files
will generate updated x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser files
ESQL 131356 should require no manual changes to these files.
QueryParam
The ES|QL parser represents ? placeholders as QueryParam records.
This is listed here only for reference - ESQL 131356 should require no change to this record.
ExpressionBuilder
ESQL 131356 will require minor changes to the ExpressionBuilder.
visitParam
The ES|QL parser converts QueryParam records into Literals in the visitParam function as indicated with ◀───. Listed here for reference - ESQL 131356 should require no change to this function.
visitLikeExpression, visitLikeListExpression
visitRlikeExpression, visitRlikeListExpression
These ExpressionBuilder functions construct LIKE and RLIKE objects for subsequent processing by ES|QL. They currently handle strings and lists of strings. For example
ESQL 131356 will require minor changes to these to support parameters and lists of parameters by delegating to appropriate helper functions. For example
Helper functions such as stringFromStringOrParameter will provide the logic ESQL 131356 needs to handle parser objects in a uniform way and raise exceptions with appropriate error messages for unsupported parameter types.
Test Changes
Unit Tests
ESQL 131356 will require minor enhancements to the unit tests which use QueryParam including the following files under x-pack/plugin/esql/src/test/java/org/elasticsearch
xpack/esql/analysis/VerifierTests.javaxpack/esql/optimizer/OptimizerVerificationTests.javaxpack/esql/action/EsqlQueryRequestTests.javaUpdate: given that the changes for ESQL 131356 all occur in the parsing stage and involve no analyzer or optimizer changes, there should be no need for additional analysis.VerifierTests or optimizer.OptimizerVerificationTests. The parser.StatementParserTests and analysis.AnalyzerTests should suffice to cover all relevant code paths. Similarly no query request tests should be needed as ESQL 131356 makes no changes to the parameter protocol or request marshalling.
CsvTest TestsAt the time of this writing it is unclear to the author if the CsvTest framework supports rest parameters. If it does, ESQL 131356 will add new queries to the where-like.csv-spec test.The CsvTests do not support rest parameters.
RestEsqlTestCase
ESQL 131356 will add new tests to RestEsqlTestCase.java file to cover use of LIKE / RLIKE with ? rest query parameter placeholders.
yamlRestTests
ESQL 131356 will add new queries to the 80_text.yml test to make use of ? rest query parameter placeholders in LIKE and RLIKE queries.
Documentation Changes
ES|QL operators | Reference
ESQL 131356 will update the documentation in the operators reference section of the ES|QL documentation generated with gradle rules which rely on annotations in the following files under x-pack/plugin/esql/src/main/java/org/elasticsearch
Planned approach
The text of ES|QL query and result examples resides in csv-spec test files such as docs.csv-spec.
These are included into the documentation by directives in the detailed description of the @FunctionInfo annotation. For example in RLike.java the directive
would reference text in the corresponding tag in docs.csv such as:
Problem
It was expected that ESQL 131356 would add tagged examples to the
docs.csv-spec but that won’t work for this feature because it would
cause the CsvTests to fail since they cannot provide a ?pattern value.
Workaround
Provide the markdown manually.
Miscellaneous
Support for “double-param” ?? parameters to allow substitution of arbitrary identifiers won’t be part of this work. If there’s a need for those we can add it in a followup change.
We may want to support array parameter values in a followup change. For example the following two queries would be equivalent if we allow an array value as a pattern.
Status
Phase 1 - identify and run tests
Update this plan, identifying tests to be updated and how each is run
Run esql :x-pack:plugin:esql:test tasks for tests to be updated
Run single node esql :x-pack:plugin:esql:qa:server:single-node:javaRestTest tasks
The commands to run these are
% ./gradlew :x-pack:plugin:esql:test \ --tests "org.elasticsearch.xpack.esql.parser.StatementParserTests" % ./gradlew :x-pack:plugin:esql:test \ --tests "org.elasticsearch.xpack.esql.analysis.AnalyzerTests" % ./gradlew :x-pack:plugin:esql:qa:server:single-node:javaRestTest \ --tests "org.elasticsearch.xpack.esql.qa.single_node.RestEsqlIT"Phase 2 - minimal feature
{ "query": "FROM base_conversion | WHERE string LIKE ?pattern | KEEP string ", "params": [{"pattern" : "f*"}] }Phase 3a - document
Update the examples in the docs.csv-spec included into the documentationx-pack/plugin/esql/qa/testFixtures/src/main/resources/docs.csv-specThis won’t work since it causes the CsvSpec tests to fail.
As a workaround manually add markdown directly in the annotations.
Update the annotations of the LIKE and RLIKE functions to indicate parameter support
The unit test generates the documentation and docs-builder renders and serves it locally.
% ./gradlew :x-pack:plugin:esql:test -Dtests.class='*Like*Tests' % docs-builder serveOpen http://localhost:3000/reference/query-languages/esql/functions-operators/operators#esql-like to view the rendered documentation.
Phase 3b - implement
Update this plan with relevant details from earlier phases
Add capabilities needed by tests and ensure tests check for them
Update parser/StatementParserTests
Identify and update other tests listed in Phase 1
Run unit test with
./gradlew :x-pack:plugin:esql:test --testsorg.elasticsearch.xpack.esql.parser.StatementParserTests.testLikeRLikeParamorg.elasticsearch.xpack.esql.analysis.AnalyzerTests.testLikeParametersRun integration test with
./gradlew :x-pack:plugin:esql:qa:server:single-node:javaRestTest —testsorg.elasticsearch.xpack.esql.qa.single_node.RestEsqlITRun integration test with
./gradlew :x-pack:plugin:esql:qa:server:single-node:yamlRestTest --testsorg.elasticsearch.xpack.esql.qa.single_node.EsqlClientYamlIT \-Dtests.rest.suite=esql/80_text --configuration-cachePhase 4 - prepare draft PR
Miscellaneous Work
Add transport version esql_like_parameter_support_tvAdd transport version check?No changes to node serialization so new transport version should not be needed.
Prepare review
Rendered docs: LIKE, RLIKE
Investigate CI/CD issues
Phase 5 - finish
Phase 6 - fix tests, update PR, invite reviewers
Update parser/StatementParserTests for LIKE list, RLIKE and RLIKE list
Update other tests for LIKE list, RLIKE and RLIKE list
Run unit test with
./gradlew :x-pack:plugin:esql:test --tests"org.elasticsearch.xpack.esql.parser.StatementParserTests.test*Param""org.elasticsearch.xpack.esql.analysis.AnalyzerTests.test*Parameters"Run integration test with
./gradlew :x-pack:plugin:esql:qa:server:single-node:javaRestTest -–testsorg.elasticsearch.xpack.esql.qa.single_node.RestEsqlITRun integration test with
./gradlew :x-pack:plugin:esql:qa:server:single-node:yamlRestTest --testsorg.elasticsearch.xpack.esql.qa.single_node.EsqlClientYamlIT \-Dtests.rest.suite=esql/80_text --configuration-cacheWrap-up