Skip to content
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

ESQL: Refactor EsqlTranslatorHandler #105230

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

alex-spies
Copy link
Contributor

Small refactoring suggested by @costin:

  • Move the expression translators into their own, dedicated class. Actual translation is not the translator handler's job; also, separating this is in line with EQL's QueryTranslator and the general ExpressionTranslators class in the QL project.
  • Replace TrivialBinaryComparison and ExpressionTranslators.BinaryComparisons by a single translator handler that delegates to ExpressionTranslators.BinaryComparisons if necessary. This is in line with other customized translators, e.g. Scalars (both ESQL's and EQL's).

Note to reviewers: Probably it's easiest to view the two commits separately; each of them is highly mechanical by itself.

Try to translate an out-of-range comparison first; if the translation
does not apply, delegate directly to
ExpressionTranslators.BinaryComparisons, rather than having two separate
rules.
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 7, 2024
@alex-spies
Copy link
Contributor Author

Maybe we also want to backport this to 8.12 - so that other potential backports come with fewer merge conflicts.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alex-spies alex-spies merged commit 7c385b0 into elastic:main Feb 14, 2024
15 checks passed
@alex-spies alex-spies deleted the refactor-esql-expr-translator branch February 14, 2024 11:09
not-napoleon added a commit that referenced this pull request Mar 12, 2024
Bbuilding on the work @alex-spies did in #105230 to start to decouple the ESQL translation layer from the QL translation layer. I did this by straight up copying the QL binary comparison code from ExpressionTranslators.BinaryComparisons to EsqlExpressionTranslators.BinaryComparisons.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants