-
Notifications
You must be signed in to change notification settings - Fork 25.3k
ESQL: Don't build can_match queries we can't push to data nodes #130210
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
base: main
Are you sure you want to change the base?
Conversation
Changes the `search_shards` api to skip sending unsupported queries, instead sending `match_all` which should make it "fail open", returning "this shard can match this query" for all unsupported queries. This is *weird*, but it's useful for systems that build the query internally inside of elasticsearch and can fall back to filtering outside of lucene. Which is *exactly* what ESQL does.
I'm creating this as a draft for the moment to get CI to run on it. If CI is happy I'll shop this around other hackers and make a unit test. |
@dnhatn and @julian-elastic, I've implemented Nhat's idea of checking transport version in the translation logic. I've grabbed #130151 for an integration test of this. |
assertResultMapForLike(includeCCSMetadata, result, columns, values, false, true); | ||
} | ||
|
||
public void testNotLikeListIndex() throws Exception { |
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.
testNotLikeListIndex will fail some of the UTs, please see the latest from my PR for fix.
I just disabled this test if it does not have like_list_on_index_fields
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.
Please copy the latest version of this file from #130019 to get all the latest fixes on the tests
var plan = plan(query, null); | ||
|
||
var filter = filterQueryForTransportNodes(TransportVersion.current(), plan); | ||
assertNull(filter); |
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.
Nit: Maybe add another test case with null transport version here and make sure the filter is not null
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.
Overall looks good. I left a comment to add another UT and fix the failing UTs by copying my fix from the correct PR (not the one you copied from originally)
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. Thanks Nik!
Passes the minimum transport version down to expressions when we convert them into queries that we'll use for can_match. Right now all this is used for is skipping the can_match from the wildcard like queries. The queries we make there aren't serializable. We'll fix that - but this should give us the levers that we need to do it in a backwards incompatible way.
Changes the `search_shards` api to skip sending unsupported queries, instead sending `match_all` which should make it "fail open", returning "this shard can match this query" for all unsupported queries.This is weird, but it's useful for systems that build the query internally inside of elasticsearch and can fall back to filtering outside of lucene. Which is exactly what ESQL does.