Skip to content

Comments

Add tests for hash collisions#842

Merged
Dandandan merged 4 commits intoapache:masterfrom
alamb:alamb/test_hash_collisions
Aug 9, 2021
Merged

Add tests for hash collisions#842
Dandandan merged 4 commits intoapache:masterfrom
alamb:alamb/test_hash_collisions

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Aug 9, 2021 •
edited
Loading

Which issue does this PR close?

Closes #841

Rationale for this change

  1. The hash implementation in ahash::RandomState is very good, and thus it is very hard to cause hash collisions in tests
  2. Since it is hard to cause hash collisions, it means we do not have test coverage of this case. This is a problem because checking for hash collisions is often required in the performance critical section of code and thus prone to being optimized out

What changes are included in this PR?

  1. Add a (off by default) feature flag that forces hash collisions
  2. Add a CI test that runs queries using those flags
  3. Updates tests to pass, and filed tickets for those that did not

Are there any user-facing changes?

No

@alamb alamb added the development-process Related to development process of DataFusion label Aug 9, 2021
@alamb alamb mentioned this pull request Aug 9, 2021
Closed
Dandandan
Dandandan approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@Dandandan Dandandan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Cool approach!
I am wondering if we can keep this in all test cases as for bigger joins / aggregates it could slow it down significantly.

Also we might want to add some explicit test for the problem (might be null-related? #843 (comment)) later.

jorgecarleitao
jorgecarleitao approved these changes Aug 9, 2021
View reviewed changes
Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

Really neat and long-lasting approach!

@Dandandan Dandandan merged commit 30cc674 into apache:master Aug 9, 2021
@Dandandan Dandandan mentioned this pull request Aug 9, 2021
Merged
@alamb alamb deleted the alamb/test_hash_collisions branch August 8, 2023 20:12
unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
@andygrove
minor: stop warning that AQEShuffleRead cannot run natively (apache#842)
befabdc 
* minor: stop warning that AQEShuffleRead cannot run natively

* address feedback

* revert a change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Dandandan Dandandan Dandandan approved these changes

+1 more reviewer

@jorgecarleitao jorgecarleitao jorgecarleitao approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

development-process Related to development process of DataFusion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add tests for hash collisions

3 participants

@alamb @Dandandan @jorgecarleitao