Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make search functions translation aware #118355
Make search functions translation aware #118355
Changes from 5 commits
7102c33
ce68659
da29b3f
5acb8d5
a195a87
667d2a6
e4ec2f3
f12b980
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think it would make sense to centralize in this interface all the pushdown logic, including
The first point is covered by PushFiltersToSource physical rule. I'm not sure if we want ad-hoc logic for each function or if we can simplify it to a single and generic behavior (ie. removing the
if (exp instanceof Term
). In both cases I'd rather have anexp instanceof TranslationAware
there, so that one day we can also let other operators/functions implement it and remove that long list ofinstanceof
s.For the second point I'm thinking about the Verifier logic around FullTextFunctions. It's a complex topic that involves some deep understanding of how commands interact with each other, I put it here for completeness but probably we can address it in a follow-up 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.
thank you for the review!
For the first point the
PushFiltersToSource
we need to look into it but I think we don't need the condition forTerm
- we removed a similar one forMatch
- see https://github.com/elastic/elasticsearch/pull/117555/files#r1858385620For the second point, agreed we can do it in a follow up - I think it would be a big enough change that we would want to review separately.
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.
Add a
@Override
annotation here.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.
If
ExpressionTranslator<? extends FullTextFunction>
is used here, the@SuppressWarnings("rawtypes")
can go away. The same applies to thetranslator()
of the fiveFullTextFunction
s.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 not used yet - but this is what we will call for the query rewrite phase - see #118106
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 this be private? And it looks better(to me) if this is after the constructor with
FunctionInfo
. The same applies to the other three -Match
,QueryString
andTerm
.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 made them private and then the
EsqlNodeSubclassTests
started failing because AFAICT they expect that the argument number from the longest public constructor to correlate with the number of args fromNodeInfo
.This to me seems to be a quirk of the
EsqlNodeSubclassTests
rather than a test catching a real bug withFullTextFunction
sThere 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 guess we could move the translators from
EsqlExpressionTranslators
but for now I just leave them as they are - I wasn't sure we need to move them. happy to hear otherwise.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.
My preference is to move them, but not in this PR - as a follow up.