Skip to content

ESQL: Fix and unmute BlockHashTests.test3BytesRefs() #127830

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
May 7, 2025

Conversation

ivancea
Copy link
Contributor

@ivancea ivancea commented May 7, 2025

Fixes #127825
Fixes #127826

After merging #127582, there was a subtle change that made calls to add(..., IntVectorBlock) to directly call add(..., IntVector). This triggered a code path in the test, throwing a NPE.

Also, updated some autogenerated files that were merged before my other branch, and weren't updated

@ivancea ivancea requested review from nik9000 and dnhatn May 7, 2025 14:19
@ivancea ivancea added >test Issues or PRs that are addressing/adding tests Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.1.0 labels May 7, 2025
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Ivan!

@idegtiarenko
Copy link
Contributor

I am a bit puzzled. Looks like changes generated files, but I do not see a change in corresponding GroupingAggregatorImplementer. Do I miss anything?

@ivancea
Copy link
Contributor Author

ivancea commented May 7, 2025

@idegtiarenko I made the changes here: #127582
And PR adding those aggs was created before I merged.

Those files are not related to this fix, I just used this PR to avoid creating another PR. Sorry for the confusion!

@ivancea ivancea enabled auto-merge (squash) May 7, 2025 14:28
@ivancea ivancea merged commit 5ea6cb5 into elastic:main May 7, 2025
17 checks passed
@ivancea ivancea deleted the esql-fix-block-hash-test-mutes branch May 7, 2025 16:29
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request May 9, 2025
Fixes elastic#127825
Fixes elastic#127826

After merging elastic#127582, there was a subtle change that made calls to `add(..., IntVectorBlock)` to directly call `add(..., IntVector)`. This triggered a code path in the test, throwing a NPE.

Also, updated some autogenerated files that were merged before my other branch, and weren't updated
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request May 12, 2025
Fixes elastic#127825
Fixes elastic#127826

After merging elastic#127582, there was a subtle change that made calls to `add(..., IntVectorBlock)` to directly call `add(..., IntVector)`. This triggered a code path in the test, throwing a NPE.

Also, updated some autogenerated files that were merged before my other branch, and weren't updated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] BlockHashTests test3BytesRefs {forcePackedHash=false} failing [CI] BlockHashTests test3BytesRefs {forcePackedHash=true} failing
5 participants