Skip to content

Move HashJoin from RawTable to HashTable #14904

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
Feb 27, 2025
Merged

Conversation

Dandandan
Copy link
Contributor

@Dandandan Dandandan commented Feb 26, 2025

Which issue does this PR close?

Rationale for this change

Use some more HashTable, so we can upgrade (and benefit from further improvements).

I tested the benchmarks, I get no regressions (makes sense, as it is using similar APIs).

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Feb 26, 2025
Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

👍🏻

self.map.allocation_info().1.size() + self.next.capacity() * size_of::<u64>()
let fixed_size = size_of::<PruningJoinHashMap>();

estimate_memory_size::<(u64, u64)>(self.map.capacity(), fixed_size).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe put a TODO here: once we updated the the latest hashbrown version, you no longer need to estimate this, see https://docs.rs/hashbrown/latest/hashbrown/struct.HashTable.html#method.allocation_size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good idea

@alamb
Copy link
Contributor

alamb commented Feb 27, 2025

Love it

@alamb alamb merged commit fc2fbb3 into apache:main Feb 27, 2025
24 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 27, 2025

Thank you @Dandandan @jayzhan211 and @crepererum

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Changes to the physical-expr crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants