Skip to content

ESQL: Fix AttributeSet#add() returning the opposite expected value #117367

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

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

ivancea
Copy link
Contributor

@ivancea ivancea commented Nov 22, 2024

Set/Collection#add() is supposed to return true if the collection changed (If it actually added something).
In this case, it must return if the old value is null.

Extracted from #114317 (Where it's being used)

@ivancea ivancea added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Nov 22, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@astefan astefan requested review from costin and removed request for nik9000 November 22, 2024 17:27
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.

I agree with this change.

I manually went over the declarations of AttributeSet to check for usages of the add method's return type. (Local var declarations, field declarations, method parameter declarations)

I only found one: AttributeSet.add's return type is used in CombineProjections. (There may be more if I overlooked them, as with every manual check.)

This means there is probably a bug there which we should investigate before merging this.

@ivancea
Copy link
Contributor Author

ivancea commented Nov 22, 2024

@alex-spies In CombineProjections we aren't using the return value (Until the Categorize PR), so all nice! Thanks for checking!
If tests are ok (And in the other PR, they were), we should be fine then :blob:

@ivancea ivancea requested a review from alex-spies November 22, 2024 17:51
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.

Argh, sorry - I was looking at your branch where you started using the return value. Of course, this change should be fine then - phew!

@nik9000
Copy link
Member

nik9000 commented Nov 22, 2024

Thanks for yanking it into it's own PR!

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.

Oopps. LGTM
P.S. Thanks for extracting it into its own PR!

@ivancea
Copy link
Contributor Author

ivancea commented Nov 25, 2024

Also updated the QL counterpart. I didn't find any usage of the add(), at all

@ivancea ivancea merged commit 3896de6 into elastic:main Nov 25, 2024
16 checks passed
@ivancea ivancea deleted the esql-attributeset-add-fix branch November 25, 2024 11:34
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

ivancea added a commit to ivancea/elasticsearch that referenced this pull request Nov 25, 2024
…lastic#117367)

Set/Collection#add() is supposed to return `true` if the collection changed (If it actually added something).
In this case, it must return if the old value is null.

Extracted from elastic#114317 (Where it's being used)
elasticsearchmachine pushed a commit that referenced this pull request Nov 25, 2024
…117367) (#117462)

Set/Collection#add() is supposed to return `true` if the collection changed (If it actually added something).
In this case, it must return if the old value is null.

Extracted from #114317 (Where it's being used)
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
…lastic#117367)

Set/Collection#add() is supposed to return `true` if the collection changed (If it actually added something).
In this case, it must return if the old value is null.

Extracted from elastic#114317 (Where it's being used)
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 >non-issue 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.

5 participants