-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Added optional parameters to QSTR ES|QL function #121787
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
Added optional parameters to QSTR ES|QL function #121787
Conversation
d15c264
to
6e70003
Compare
Warning It looks like this PR modifies one or more |
6e70003
to
95fb7a0
Compare
Warning It looks like this PR modifies one or more |
95fb7a0
to
66ce0bc
Compare
Warning It looks like this PR modifies one or more |
Still TODO: add randomized test, take care of documentation warnings |
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryString.java
Show resolved
Hide resolved
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
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.
This is looking good! 💯
Some things I miss:
- CSV tests for having an end-to-end test of some option
- Some testing in
MatchTests
that adds the function named param types
I left some other comments to address, but this is almost done.
|=== | ||
name | types | description | ||
fuzziness | [keyword] | Maximum edit distance allowed for matching. | ||
auto_generate_synonyms_phrase_query | [boolean] | If true, match phrase queries are automatically created for multi-term synonyms. |
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 realize now that the docs generate options unsorted - we should probably change AbstractFunctionTestCase so it outputs an ordered options list.
No need to do in this PR, can be a follow up.
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryString.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
Outdated
Show resolved
Hide resolved
String optionName = allowedOptions.getKey(); | ||
DataType optionType = allowedOptions.getValue(); | ||
// Check every possible type for the option - we'll try to convert it to the expected type | ||
for (DataType currentType : optionTypes) { |
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 probably extract this testing code to a separate method so both match
and qstr
can reuse it
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.
This belongs to QL and not ESQL - I'd say let's keep this out of the scope for this PR
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 understand that. Since I'm touching a related area, and since there was a TODO to update the constants to fields, I thought it is a good idea to make the change now that the query string builder constants are public.
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryString.java
Outdated
Show resolved
Hide resolved
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryString.java
Show resolved
Hide resolved
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're missing the additional named param types in the tests, so the named parameters
param is added to the function types. See how match did it here.
...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java
Outdated
Show resolved
Hide resolved
…expression/function/fulltext/QueryString.java Co-authored-by: Carlos Delgado <[email protected]>
Warning It looks like this PR modifies one or more |
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.
LGTM - Good work! 💯
There's a pending comment about adding default values to the options in the docs
@svilen-mihaylov-elastic I'm adding backports for 8.19 and 9.0, plus |
@elasticsearchmachine test this |
Hi @svilen-mihaylov-elastic, I've updated the changelog YAML for you. |
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.
LGTM, thank you @svilen-mihaylov-elastic!
💔 Backport failed
You can use sqren/backport to manually backport by running |
Adds options to QSTR function. elastic#118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes elastic#120933
Adds options to QSTR function. elastic#118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes elastic#120933 (cherry picked from commit ee4bcac) # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
Adds options to QSTR function. elastic#118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes elastic#120933 (cherry picked from commit ee4bcac) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/qstr-function.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java
…5114) * Added optional parameters to QSTR ES|QL function (#121787) Adds options to QSTR function. #118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes #120933 (cherry picked from commit ee4bcac) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/qstr-function.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java * [CI] Auto commit changes from spotless * Fix compilation error * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
…5112) * Added optional parameters to QSTR ES|QL function (#121787) Adds options to QSTR function. #118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes #120933 (cherry picked from commit ee4bcac) # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java * [CI] Auto commit changes from spotless * Fix compilation error * Fix missing import * getFirst() -> get(0) * Make method public * Make asBuilder public in subclasses * Revert "Make asBuilder public in subclasses" This reverts commit f444aa6. * Revert "Make method public" This reverts commit a1d9f56. * .asQueryBuilder() -> .toQueryBuilder() * Remove extraneous change which sneaked in during backport --------- Co-authored-by: elasticsearchmachine <[email protected]>
Adds options to QSTR function.
#118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL.
Closes #120933