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

ESQL: Grammar - FROM METADATA no longer require [] #105221

Merged
merged 13 commits into from Feb 8, 2024
Merged

ESQL: Grammar - FROM METADATA no longer require [] #105221

merged 13 commits into from Feb 8, 2024

Conversation

costin
Copy link
Member

@costin costin commented Feb 6, 2024

Remove usage of [ ] through-out the grammar, in this case inside
FROM METADATA.

@costin
ESQL: Grammar - FROM METADATA no longer require []
7c1f091 
Remove usage of [ ] through-out the grammar, in this case inside
 FROM METADATA.
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 6, 2024

Documentation preview:

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 6, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @costin, I've created a changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

@elasticsearchmachine
Copy link
Collaborator

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

bpintea
bpintea approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

Lgtm

luigidellaquila
luigidellaquila approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@luigidellaquila luigidellaquila left a comment •
edited

Choose a reason for hiding this comment

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

Thanks Costin, LGTM.

For the failing tests (all the CSV tests that still use square brackets for metadata and that now expect a warning), my feeling is that there is no solution apart from skipping them for v < 8.13: warnings are checked as mandatory, so the tests will always fail in old versions, where the warning is not emitted

astefan
astefan approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM. Left some minor comments about wording.

docs/changelog/105221.yaml Outdated Show resolved Hide resolved
docs/changelog/105221.yaml Outdated Show resolved Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
if (deprecatedContext != null) {
var s = source(deprecatedContext).source();
addWarning(
"Line {}:{}: Square brackets '[]' need to be removed in FROM METADATA declaration",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Line {}:{}: Square brackets '[]' need to be removed in FROM METADATA declaration",
"Line {}:{}: Square brackets '[]' need to be removed from FROM METADATA declaration",

@alex-spies alex-spies mentioned this pull request Feb 7, 2024
Merged
@costin
Copy link
Member Author

costin commented Feb 8, 2024

Thanks for the reviews folks - I've updated the test and raised #105267

fang-xing-esql
fang-xing-esql approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@fang-xing-esql fang-xing-esql left a comment

Choose a reason for hiding this comment

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

LGTM.

@dej611 dej611 mentioned this pull request Feb 8, 2024
Merged
1 task
@costin costin merged commit fca3fc8 into elastic:main Feb 8, 2024
14 checks passed
@costin costin deleted the esql/metadata-grammar branch February 8, 2024 15:03
dej611 added a commit to elastic/kibana that referenced this pull request Feb 9, 2024
@dej611
[ES|QL] New sync with ES changes (#176283)
7bbea56 
## Summary

Sync with elastic/elasticsearch#104958 for
support of builtin fn in STATS
  * validation ✅ 
  * autocomplete ✅ 
  * also fixed `STATS BY <field>` syntax


![new_stats](https://github.com/elastic/kibana/assets/924948/735f9842-b1d3-4aa0-9d51-4b2f9b136ed3)


Sync with elastic/elasticsearch#104913 for new
`log` function
  * validation ✅  - also warning for negative values
  * autocomplete ✅ 

![add_log](https://github.com/elastic/kibana/assets/924948/146b945d-a23b-45ec-9df2-2d2b291e883b)

Sync with elastic/elasticsearch#105064 for
removal of `PROJECT` command
  * validation ✅  (both new and legacy syntax supported)
  * autocomplete ✅  (will only suggest new syntax)


![remove_project](https://github.com/elastic/kibana/assets/924948/b6f40afe-a26d-4917-b7a1-d8ae97c5368b)

Sync with elastic/elasticsearch#105221 for
removal of mandatory brackets for `METADATA` command option
* validation ✅ (added warning deprecation message when using brackets)
  * autocomplete ✅ 

![fix_metadata](https://github.com/elastic/kibana/assets/924948/c65db176-dd94-45f3-9524-45453e62f51a)


Sync with elastic/elasticsearch#105224 for
change of syntax for ENRICH ccq mode
  * validation ✅ 
* autocomplete ✅ (not directly promoted, the user has to type `_` to
trigger it)
  * hover ✅ 
  * code actions ✅ 

![fix_ccq_enrich](https://github.com/elastic/kibana/assets/924948/0900edd9-a0a7-4ac8-bc12-e39a72359984)

![fix_ccq_enrich_2](https://github.com/elastic/kibana/assets/924948/74b0908f-d385-4723-b3d4-c09108f47a73)


Do not merge until those 5 get merged.

Additional things in this PR:
* Added more tests for `callbacks` not passed scenario
  * covered more cases like those with `dissect`
* Added more tests for signature params number (calling a function with
an extra arg should return an error)
* Cleaned up some more unused code
* Improved messages on too many arguments for functions

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
@costin costin mentioned this pull request Feb 12, 2024
Closed
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@dej611 @CoenWarmer
[ES|QL] New sync with ES changes (elastic#176283)
7517d34 
## Summary

Sync with elastic/elasticsearch#104958 for
support of builtin fn in STATS
  * validation ✅ 
  * autocomplete ✅ 
  * also fixed `STATS BY <field>` syntax


![new_stats](https://github.com/elastic/kibana/assets/924948/735f9842-b1d3-4aa0-9d51-4b2f9b136ed3)


Sync with elastic/elasticsearch#104913 for new
`log` function
  * validation ✅  - also warning for negative values
  * autocomplete ✅ 

![add_log](https://github.com/elastic/kibana/assets/924948/146b945d-a23b-45ec-9df2-2d2b291e883b)

Sync with elastic/elasticsearch#105064 for
removal of `PROJECT` command
  * validation ✅  (both new and legacy syntax supported)
  * autocomplete ✅  (will only suggest new syntax)


![remove_project](https://github.com/elastic/kibana/assets/924948/b6f40afe-a26d-4917-b7a1-d8ae97c5368b)

Sync with elastic/elasticsearch#105221 for
removal of mandatory brackets for `METADATA` command option
* validation ✅ (added warning deprecation message when using brackets)
  * autocomplete ✅ 

![fix_metadata](https://github.com/elastic/kibana/assets/924948/c65db176-dd94-45f3-9524-45453e62f51a)


Sync with elastic/elasticsearch#105224 for
change of syntax for ENRICH ccq mode
  * validation ✅ 
* autocomplete ✅ (not directly promoted, the user has to type `_` to
trigger it)
  * hover ✅ 
  * code actions ✅ 

![fix_ccq_enrich](https://github.com/elastic/kibana/assets/924948/0900edd9-a0a7-4ac8-bc12-e39a72359984)

![fix_ccq_enrich_2](https://github.com/elastic/kibana/assets/924948/74b0908f-d385-4723-b3d4-c09108f47a73)


Do not merge until those 5 get merged.

Additional things in this PR:
* Added more tests for `callbacks` not passed scenario
  * covered more cases like those with `dissect`
* Added more tests for signature params number (calling a function with
an extra arg should return an error)
* Cleaned up some more unused code
* Improved messages on too many arguments for functions

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@dej611 @CoenWarmer
[ES|QL] New sync with ES changes (elastic#176283)
731dfe9 
## Summary

Sync with elastic/elasticsearch#104958 for
support of builtin fn in STATS
  * validation ✅ 
  * autocomplete ✅ 
  * also fixed `STATS BY <field>` syntax


![new_stats](https://github.com/elastic/kibana/assets/924948/735f9842-b1d3-4aa0-9d51-4b2f9b136ed3)


Sync with elastic/elasticsearch#104913 for new
`log` function
  * validation ✅  - also warning for negative values
  * autocomplete ✅ 

![add_log](https://github.com/elastic/kibana/assets/924948/146b945d-a23b-45ec-9df2-2d2b291e883b)

Sync with elastic/elasticsearch#105064 for
removal of `PROJECT` command
  * validation ✅  (both new and legacy syntax supported)
  * autocomplete ✅  (will only suggest new syntax)


![remove_project](https://github.com/elastic/kibana/assets/924948/b6f40afe-a26d-4917-b7a1-d8ae97c5368b)

Sync with elastic/elasticsearch#105221 for
removal of mandatory brackets for `METADATA` command option
* validation ✅ (added warning deprecation message when using brackets)
  * autocomplete ✅ 

![fix_metadata](https://github.com/elastic/kibana/assets/924948/c65db176-dd94-45f3-9524-45453e62f51a)


Sync with elastic/elasticsearch#105224 for
change of syntax for ENRICH ccq mode
  * validation ✅ 
* autocomplete ✅ (not directly promoted, the user has to type `_` to
trigger it)
  * hover ✅ 
  * code actions ✅ 

![fix_ccq_enrich](https://github.com/elastic/kibana/assets/924948/0900edd9-a0a7-4ac8-bc12-e39a72359984)

![fix_ccq_enrich_2](https://github.com/elastic/kibana/assets/924948/74b0908f-d385-4723-b3d4-c09108f47a73)


Do not merge until those 5 get merged.

Additional things in this PR:
* Added more tests for `callbacks` not passed scenario
  * covered more cases like those with `dissect`
* Added more tests for signature params number (calling a function with
an extra arg should return an error)
* Cleaned up some more unused code
* Improved messages on too many arguments for functions

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
@dej611 @fkanout
[ES|QL] New sync with ES changes (elastic#176283)
953209e 
## Summary

Sync with elastic/elasticsearch#104958 for
support of builtin fn in STATS
  * validation ✅ 
  * autocomplete ✅ 
  * also fixed `STATS BY <field>` syntax


![new_stats](https://github.com/elastic/kibana/assets/924948/735f9842-b1d3-4aa0-9d51-4b2f9b136ed3)


Sync with elastic/elasticsearch#104913 for new
`log` function
  * validation ✅  - also warning for negative values
  * autocomplete ✅ 

![add_log](https://github.com/elastic/kibana/assets/924948/146b945d-a23b-45ec-9df2-2d2b291e883b)

Sync with elastic/elasticsearch#105064 for
removal of `PROJECT` command
  * validation ✅  (both new and legacy syntax supported)
  * autocomplete ✅  (will only suggest new syntax)


![remove_project](https://github.com/elastic/kibana/assets/924948/b6f40afe-a26d-4917-b7a1-d8ae97c5368b)

Sync with elastic/elasticsearch#105221 for
removal of mandatory brackets for `METADATA` command option
* validation ✅ (added warning deprecation message when using brackets)
  * autocomplete ✅ 

![fix_metadata](https://github.com/elastic/kibana/assets/924948/c65db176-dd94-45f3-9524-45453e62f51a)


Sync with elastic/elasticsearch#105224 for
change of syntax for ENRICH ccq mode
  * validation ✅ 
* autocomplete ✅ (not directly promoted, the user has to type `_` to
trigger it)
  * hover ✅ 
  * code actions ✅ 

![fix_ccq_enrich](https://github.com/elastic/kibana/assets/924948/0900edd9-a0a7-4ac8-bc12-e39a72359984)

![fix_ccq_enrich_2](https://github.com/elastic/kibana/assets/924948/74b0908f-d385-4723-b3d4-c09108f47a73)


Do not merge until those 5 get merged.

Additional things in this PR:
* Added more tests for `callbacks` not passed scenario
  * covered more cases like those with `dissect`
* Added more tests for signature params number (calling a function with
an extra arg should return an error)
* Cleaned up some more unused code
* Improved messages on too many arguments for functions

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luigidellaquila luigidellaquila luigidellaquila approved these changes

@astefan astefan astefan approved these changes

@bpintea bpintea bpintea approved these changes

@fang-xing-esql fang-xing-esql fang-xing-esql approved these changes

@alex-spies alex-spies Awaiting requested review from alex-spies

Assignees
No one assigned
Labels
:Analytics/Compute Engine Analytics in ES|QL >breaking ES|QL-ui Impacts ES|QL UI Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@costin @elasticsearchmachine @luigidellaquila @astefan @bpintea @fang-xing-esql