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

Remove square brackets in FROM METADATA declaration #196991

Merged
merged 12 commits into from
Oct 31, 2024

Conversation

kyracho
Copy link
Contributor

@kyracho kyracho commented Oct 21, 2024

Summary

Hello, this PR addresses the deprecation of square brackets in FROM METADATA declarations in Elasticsearch queries, in preparation for Elasticsearch 9.0. Closes #196988

The changes involve removing square brackets around metadata fields (e.g., [metadata _id] becomes metadata _id) across various parts of the codebase. No functional changes to the UE, only internal query syntax updates.

Checklist

For maintainers

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Hey, thanx a lot for the contribution. I have added some comments which need to be addressed.

When you do the changes run again the validation.test.ts to update the packages/kbn-esql-validation-autocomplete/src/validation/esql_validation_meta_tests.json file.

@stratoula stratoula added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana labels Oct 21, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-esql (Team:ESQL)

@kyracho
Copy link
Contributor Author

kyracho commented Oct 23, 2024

Hey, thanx a lot for the contribution. I have added some comments which need to be addressed.

Hi @stratoula, thanks so much for your feedback! I’ll make sure to implement the changes according to your suggestions. I really appreciate your patience!

@stratoula
Copy link
Contributor

/ci

@stratoula
Copy link
Contributor

@kyracho really appreciate it. Can you take care of the unit tests failures too? When done ping me to rerun the CI 🙌

@kyracho
Copy link
Contributor Author

kyracho commented Oct 30, 2024

Hi, I noticed I don’t have access to view the Buildkite logs on this PR, which is making it difficult to troubleshoot the failures independently. Could I be granted access to the Buildkite logs to streamline the process, or could someone share the relevant logs for the test failures? Thanks so much for the help!

@stratoula
Copy link
Contributor

U r right, I cant grant you access to the buildkite but here they are

image

If you ran the validation.test.ts file I told you above you will see them

@kyracho
Copy link
Contributor Author

kyracho commented Oct 30, 2024

@stratoula thanks for your response! I tried running the specific Jest test locally using
yarn test:jest packages/kbn-esql-validation-autocomplete/src/validation/validation.test.ts
and it passes without issues:

Screenshot 2024-10-30 at 12 25 18 AM

I also looked for a way to run all Jest tests in the codebase but couldn’t find a command for that. Could you tell me how to run the full suite, or if there's a specific way to narrow down the tests?

@stratoula
Copy link
Contributor

Try this

node scripts/jest --config packages/kbn-esql-validation-autocomplete/jest.config.js packages/kbn-esql-validation-autocomplete/src/validation

@kyracho
Copy link
Contributor Author

kyracho commented Oct 30, 2024

Try this

node scripts/jest --config packages/kbn-esql-validation-autocomplete/jest.config.js packages/kbn-esql-validation-autocomplete/src/validation

I was able to reproduce the Jest test failures — thank you! :) For the FTR Configs #33 error, could you point me to the specific test or config I should run to replicate it locally? Thanks in advance!

@stratoula
Copy link
Contributor

This sees unrelated with your changes so I assume it is just a flaky test, lets fix the unit tests and rerun the CI to see if they will fail again

@kyracho
Copy link
Contributor Author

kyracho commented Oct 30, 2024

@stratoula thanks for your help!

I adjusted validation.command.from.ts and validation.ccs.test.ts to pass the unit tests:

Screenshot 2024-10-30 at 4 42 06 AM

Thanks for being patient as I'm learning a lot as I go!

@stratoula
Copy link
Contributor

Awesome @kyracho, we appreciate the help! Let me re-run the CI 🤞

@stratoula
Copy link
Contributor

@nkhristinin can you take a look on these cypress tests failing here? They seem irrelevant with the change and they are just flaky. Can you confirm?

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

ES|QL changes LGTM, thanx for clearing this up and for your contribution!

Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Changes in Data Discovery tests LGTM 👍

@stratoula
Copy link
Contributor

@elasticmachine run docs-build

@kyracho
Copy link
Contributor Author

kyracho commented Oct 30, 2024

@stratoula TYSM! <3

@stratoula
Copy link
Contributor

/ci

@stratoula
Copy link
Contributor

stratoula commented Oct 31, 2024

I have confirmed that the test failing is irrelevant, I will re-run the CI and merge

@stratoula
Copy link
Contributor

@elasticmachine run docs-build

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
esql 180.5KB 180.4KB -126.0B
unifiedSearch 353.2KB 353.1KB -126.0B
total -252.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
kbnUiSharedDeps-srcJs 3.4MB 3.4MB -119.0B

History

@stratoula stratoula merged commit e3b8ccf into elastic:main Oct 31, 2024
43 checks passed
@kyracho kyracho deleted the remove_square_brackets branch October 31, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting 💝community Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:ESQL ES|QL related features in Kibana v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[9.0][Deprecations][Detection Engine] Square brackets '[]' need to be removed in FROM METADATA declaration
6 participants