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: Clone ql for esql #108773

Merged
merged 25 commits into from
May 22, 2024
Merged

Conversation

alex-spies
Copy link
Contributor

@alex-spies alex-spies commented May 17, 2024

Part of #106679

  • Copy the ql project into a different project just for esql, call it esql-core.
  • Make esql depend only on the latter.
  • Fix EsqlNodeSubclassTests; I'm confused why this didn't bite us earlier.
  • Update the warning regexes in some csv tests as the exceptions have other package names now.

Note to reviewers: Exclude the first commit when viewing the diff, as that contains only the actual copying of ql. The remaining commits are the actually meaningful ones. The build.gradle files probably require the most attention.

@alex-spies alex-spies marked this pull request as ready for review May 17, 2024 14:20
@alex-spies alex-spies requested a review from a team as a code owner May 17, 2024 14:20
@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 May 17, 2024
@alex-spies
Copy link
Contributor Author

Also kudos to @luigidellaquila - he made the first attempt in #106079, which however included renaming the namespaces and fixing up any imports.

Copy link
Contributor

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

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

Thanks Alex!
Just a couple of observations

This needs to be changed probably

https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/ml/qa/native-multi-node-tests/build.gradle#L18

It was not in my PR because it was added just one week after that.

This mixes EQL and ESQL, not sure if it can lead to classpath issues

https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/security/qa/consistency-checks/build.gradle#L16

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.

Since we don't aim to share the QL clone outside ESQL, why use it as a separate module instead of moving it directly into ESQL?

@alex-spies
Copy link
Contributor Author

Since we don't aim to share the QL clone outside ESQL, why use it as a separate module instead of moving it directly into ESQL?

Moving that into the esql plugin directly would be more elegant. We could do that in a follow up if desired; I'd prefer not to do it in this PR, as that change is a bit less mechanical than just copying all the things, so more could go wrong.

@luigidellaquila
Copy link
Contributor

luigidellaquila commented May 21, 2024

Moving that into the esql plugin directly would be more elegant. We could do that in a follow up if desired; I'd prefer not to do it in this PR, as that change is a bit less mechanical than just copying all the things, so more could go wrong.

It could be less obvious than it seems: today Compute depends on QL, and we don't want it to depend on ESQL (but if I remember well, the dependencies were only on tests, so probably we can find an acceptable solution)

@alex-spies
Copy link
Contributor Author

Thanks for the reviews @costin , @luigidellaquila , @breskeby and @rjernst !

I addressed your remarks, all should be good now. In particular, moved the esql-core classes into an own package, xpack.plugin.esql.core.

We can move the contents of esql-core into just esql later - hopefully placing the classes in esql.core will mean we won't have to update all the imports yet another time then.

Copy link
Contributor

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

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

Still a couple of fixes to make the CI happy (MlNativeIntegTestCase and EvalBenchmark), but apart from that, LGTM

Thanks Alex!!!

As a follow-up (this PR is big enough already), we'll have to double-check that the code generator still respects the code style (eg. the order of imports)

This cannot be compileOnly; otherwise some tests fail because the
SingleValueQuery class cannot be loaded.
This reverts commit b4d92bd.

I was wrong :D
These need `esql-core`. Previously, these depended on `ql` and we
somehow didn't see them fail, maybe because `eql` and `sql` had the
class loader load the required classes.
@alex-spies alex-spies added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 22, 2024
@elasticsearchmachine elasticsearchmachine merged commit 16a5d24 into elastic:main May 22, 2024
15 checks passed
@alex-spies alex-spies deleted the clone-ql-for-esql branch May 22, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants