Skip to content

Conversation

tteofili
Copy link
Contributor

@tteofili tteofili commented Dec 11, 2024

Drop checks in ES|QL Verifier that ensured _score is a reserved attribute.
These checks are too restrictive, in addition to that relaxing them would make handling of _score consistent with other (metadata) attributes and fields.

fixes #118460

Copy link
Contributor

Documentation preview:

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 labels Dec 11, 2024
@tteofili tteofili added :Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >enhancement v8.18.0 labels Dec 11, 2024
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed needs:triage Requires assignment of a team area label labels Dec 11, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @tteofili, I've created a changelog YAML for you.

Copy link
Contributor

@ioanatia ioanatia left a comment

Choose a reason for hiding this comment

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

The changes look right with me!
Can we get a CSV test just to prevent any future regressions with allowing _score to be overwritten?
Something like what you had in the test before: "from foo metadata _score | where qstr(\"bar\") | eval _score = _score + 1"
and another one where _score receives a different type:

from foo metadata _score | where qstr(\"bar\") | eval _score = "foobar"

with that, I think this should be good to go!

@ioanatia
Copy link
Contributor

also what would happen if _score is an actual indexed field? have we tested that?
we don't have to fix it here, but can be a follow up in the meta issue #116599

@tteofili
Copy link
Contributor Author

also what would happen if _score is an actual indexed field? have we tested that?

added an item in #116599

an we get a CSV test just to prevent any future regressions with allowing _score to be overwritten?

added a couple of tests for score manipulation and override in 0ff6fb3

@ioanatia
Copy link
Contributor

ioanatia commented Dec 11, 2024

an we get a CSV test just to prevent any future regressions with allowing _score to be overwritten?

added a couple of tests for score manipulation and override in 0ff6fb3

I did not realize this would actually create a problem for bwc 8.18 tests - feel free to remove them and then we can add them later after the changes make it in 8.18.
The alternatives are:

  • mute the tests you just added and unmute once these changes make it in 8.18
  • add a new EsqlCapability to signal that _score is no longer reserved - this is too heavy, let's not do it

@elasticsearchmachine
Copy link
Collaborator

Hi @tteofili, I've updated the changelog YAML for you.

@tteofili tteofili merged commit 7573312 into elastic:main Dec 12, 2024
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 118435

- class: "org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT"
method: "test {scoring.*}"
issue: https://github.com/elastic/elasticsearch/issues/117641
- class: "org.elasticsearch.xpack.esql.qa.mixed.MultilusterEsqlSpecIT"
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a typo here - also looks like a duplicate with the previous muted test (line 180)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh no! dummy PR to fix that #118665

tteofili added a commit to tteofili/elasticsearch that referenced this pull request Dec 12, 2024
@tteofili
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

Questions ?

Please refer to the Backport tool documentation

tteofili added a commit that referenced this pull request Dec 13, 2024
maxhniebergall pushed a commit to maxhniebergall/elasticsearch that referenced this pull request Dec 16, 2024
maxhniebergall pushed a commit to maxhniebergall/elasticsearch that referenced this pull request Dec 16, 2024
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-backport Automatically create backport pull requests when merged backport pending >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

allow _score in ES|QL to be manipulated

4 participants