-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
SQL: Fix issue with options for QUERY() and MATCH(). #33828
Conversation
Previously multiple comma separated lists of options where not recognized correctly which resulted in only the last of them to be taked into account, e.g.: For the following query: SELECT * FROM test WHERE QUERY('search', 'default_field=foo', 'default_operator=and')" only the `default_operator=and` was finally passed to the ES query. Fixes: elastic#32602
| predicated #booleanDefault | ||
| left=booleanExpression operator=AND right=booleanExpression #logicalBinary | ||
| left=booleanExpression operator=OR right=booleanExpression #logicalBinary | ||
; | ||
|
||
matchQueryOptions |
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 chose to use this separate marker to avoid confusion in the ExpressionBuilder
with string()
containing also the queryString
argument which had to be skipped.
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.
Is it possible to not have it as a top-level declaration and use something like options=(',' string)*
instead?
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.
Nope, unfortunately not, because of the *
wildcard.
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.
Left a couple of minor comments. LGTM otherwise.
| predicated #booleanDefault | ||
| left=booleanExpression operator=AND right=booleanExpression #logicalBinary | ||
| left=booleanExpression operator=OR right=booleanExpression #logicalBinary | ||
; | ||
|
||
matchQueryOptions |
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.
Is it possible to not have it as a top-level declaration and use something like options=(',' string)*
instead?
...l/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/fulltext/FullTextUtils.java
Show resolved
Hide resolved
Pinging @elastic/es-search-aggs |
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
Previously multiple comma separated lists of options where not recognized correctly which resulted in only the last of them to be taked into account, e.g.: For the following query: SELECT * FROM test WHERE QUERY('search', 'default_field=foo', 'default_operator=and')" only the `default_operator=and` was finally passed to the ES query. Fixes: #32602
Backported to |
* master: (46 commits) Fixing assertions in integration test (elastic#33833) [CCR] Rename idle_shard_retry_delay to poll_timout in auto follow patterns (elastic#33821) HLRC: Delete ML calendar (elastic#33775) Move DocsStats into Engine (elastic#33835) [Docs] Clarify accessing Date methods in painless (elastic#33560) add elasticsearch-shard tool (elastic#32281) Cut over to unwrap segment reader (elastic#33843) SQL: Fix issue with options for QUERY() and MATCH(). (elastic#33828) Emphasize that filesystem-level backups don't work (elastic#33102) Use the global doc id to generate a random score (elastic#33599) Add minimal sanity checks to custom/scripted similarities. (elastic#33564) Profiler: Don’t profile NEXTDOC for ConstantScoreQuery. (elastic#33196) [CCR] Change FollowIndexAction.Request class to be more user friendly (elastic#33810) SQL: day and month name functions tests locale providers enforcement (elastic#33653) TESTS: Set SO_LINGER = 0 for MockNioTransport (elastic#32560) Test: Relax jarhell gradle test (elastic#33787) [CCR] Fail with a descriptive error if leader index does not exist (elastic#33797) Add ES version 6.4.2 (elastic#33831) MINOR: Remove Some Dead Code in Scripting (elastic#33800) Ensure realtime `_get` and `_termvectors` don't run on the network thread (elastic#33814) ...
Previously multiple comma separated lists of options where not
recognized correctly which resulted in only the last of them
to be taked into account, e.g.:
For the following query:
SELECT * FROM test WHERE QUERY('search', 'default_field=foo', 'default_operator=and')"
only the
default_operator=and
was finally passed to the ES query.Fixes: #32602