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.

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

Introduce HashMap and HashSet type aliases #13236

Merged
merged 6 commits into from
Nov 5, 2024

Conversation

drauschenbach
Copy link
Contributor

@drauschenbach drauschenbach commented Nov 3, 2024

Which issue does this PR close?

Closes #677.

Rationale for this change

Centralize the recommended default implementation of HashMap and HashSet.

What changes are included in this PR?

  • All references to hashbrown::HashMap are now unified under the datafusion_common::HashMap type alias.
  • All references to hashbrown::HashSet are now unified under the datafusion_common::HashSet type alias.
  • Some additional references to std::collections::HashMap and std::collections::HashSet are replaced with the type alias, where public contracts were not involved, and where tests did not break.

Are these changes tested?

Yes, on an Apple M1.

$ cargo test
...
All modules: test result: ok.

Are there any user-facing changes?

Hopefully not. All tests and public contracts are left untouched, where std::collections:: types are the idiomatic platform default.

@drauschenbach drauschenbach marked this pull request as draft November 3, 2024 17:12
@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate substrait common Related to common crate execution Related to the execution crate functions labels Nov 3, 2024
@github-actions github-actions bot added the sql SQL Planner label Nov 3, 2024
collections::{HashMap, HashSet},
sync::Arc,
};
use std::{collections::HashSet, sync::Arc};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could do the same to HashSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@drauschenbach drauschenbach changed the title Hashmap type alias HashMap type alias Nov 3, 2024
@drauschenbach drauschenbach changed the title HashMap type alias Introduce a HashMap type alias Nov 3, 2024
@drauschenbach drauschenbach changed the title Introduce a HashMap type alias Introduce HashMap and HashSet type aliases Nov 3, 2024
@drauschenbach drauschenbach marked this pull request as ready for review November 3, 2024 23:48
@Dandandan Dandandan merged commit 2e52580 into apache:main Nov 5, 2024
27 checks passed
@Dandandan
Copy link
Contributor

Thank you @drauschenbach

@drauschenbach drauschenbach deleted the hashmap-type-alias branch November 5, 2024 14:10
findepi pushed a commit to sdf-labs/arrow-datafusion that referenced this pull request Nov 25, 2024
* Unite all references to hashbrown::HashMap by using a common type definition

* Replace some use of std::collections::HashMap with hashbrown::HashMap

* Replace some use of std::collections::HashMap with hashbrown::HashMap

* Replace some use of std::collections::HashMap with hashbrown::HashMap

* Unite all references to hashbrown::HashSet by using a common type definition

* Replace some use of std::collections::HashSet with hashbrown::HashSet
findepi pushed a commit to sdf-labs/arrow-datafusion that referenced this pull request Nov 26, 2024
* Unite all references to hashbrown::HashMap by using a common type definition

* Replace some use of std::collections::HashMap with hashbrown::HashMap

* Replace some use of std::collections::HashMap with hashbrown::HashMap

* Replace some use of std::collections::HashMap with hashbrown::HashMap

* Unite all references to hashbrown::HashSet by using a common type definition

* Replace some use of std::collections::HashSet with hashbrown::HashSet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate core Core DataFusion crate execution Related to the execution crate functions logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sql SQL Planner substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a single HashMap implementation consistently across the code base
3 participants