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.

Sign up for GitHub

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

Jump to bottom

ES|QL: register TABLES_TYPES capability and fix CSV tests #109926

Merged
merged 8 commits into from
Jun 20, 2024
Merged

ES|QL: register TABLES_TYPES capability and fix CSV tests #109926

merged 8 commits into from
Jun 20, 2024

Conversation

luigidellaquila
Copy link
Contributor

Registering TABLES_TYPES capability, that was created (but not registered) in #109489

Also using it as a condition for lookup.csv-spec tests.
These tests failed badly in Serverless multi-cluster due to different output format in two different nodes.

Probably (if I got it right) the YAML tests that were using this capability were not running at all. This PR should re-enable them.

@luigidellaquila luigidellaquila added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL labels Jun 19, 2024
@elasticsearchmachine elasticsearchmachine added v8.15.0 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Jun 19, 2024
@elasticsearchmachine
Copy link
Collaborator

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

alex-spies
alex-spies approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Thanks Luigi!

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java Outdated
Comment on lines 94 to 95
caps.add(LOOKUP_COMMAND);
caps.add(TABLES_TYPES);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd argue that we should actually remove LOOKUP_COMMAND, as TABLES_TYPES introduced a breaking change and should therefore be treated as a LOOKUP_COMMAND_V2.

If we do this, we should align the yaml tests as well - these currently require both tables_types AND lookup_command and would be skipped if we removed LOOKUP_COMMAND.

For our test suites, it should make no difference if we have both caps here or only TABLES_TYPES; however, having both capabilities is like a node was saying "I can handle both the old and the new lookup!", which is wrong and could lead to failures in CCQ-scenarios where the remote cluster is newer than the local cluster.

@alex-spies alex-spies mentioned this pull request Jun 19, 2024
Closed
@luigidellaquila luigidellaquila mentioned this pull request Jun 19, 2024
Merged
nik9000
nik9000 approved these changes Jun 19, 2024
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Yeah, it does make sense to swap it to the new one. It's nice to have the flexibility to just do whatever we need to do with these capabilities.

@luigidellaquila luigidellaquila added auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Jun 20, 2024
@luigidellaquila
Copy link
Contributor Author

Thanks for the feedback folks!

@luigidellaquila luigidellaquila added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jun 20, 2024
@luigidellaquila
Copy link
Contributor Author

@elasticmachine update branch

@luigidellaquila
Copy link
Contributor Author

@elasticmachine update branch

@luigidellaquila
Copy link
Contributor Author

@elasticmachine update branch

@luigidellaquila
Copy link
Contributor Author

@elasticmachine update branch

@luigidellaquila
Copy link
Contributor Author

@elasticmachine update branch

@luigidellaquila
Copy link
Contributor Author

@elasticmachine update branch

@elasticsearchmachine elasticsearchmachine merged commit 7dd7267 into elastic:main Jun 20, 2024
15 checks passed
@luigidellaquila luigidellaquila deleted the esql/bwc_lookup_table_types branch June 20, 2024 15:54
elasticsearchmachine pushed a commit that referenced this pull request Jun 21, 2024
@luigidellaquila
ES|QL: make capabilities an enum (#109935)
57f4870 
So that registration is automatic.

Let's merge it after
#109926 to make sure all
the tests are good
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

@alex-spies alex-spies alex-spies approved these changes

Assignees
No one assigned
Labels
:Analytics/ES|QL AKA ESQL auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@luigidellaquila @elasticsearchmachine @nik9000 @alex-spies @elasticmachine