Update hashbrown requirement from 0.14.2 to 0.15.0 #6493
Conversation
github-actions
bot
added
parquet
| @@ -29,7 +29,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> { | |||
| // protoc in unbuntu builder needs this option | |||
| .protoc_arg("--experimental_allow_proto3_optional") | |||
| .out_dir("src") | |||
| .compile_with_config(prost_config(), &[proto_path], &[proto_dir])?; | |||
There was a problem hiding this comment.
This is a tonic deprecation warning, fixed here. I can move to another PR if a problem.
wiedld
force-pushed
the
wiedld/update-hashbrown
branch
from
310030b
to
df71a7d
Compare
|
From the change log:
So if we don't fix that during the upgrade, we should track the migration in a ticket so we're not getting into a pickle when that functionality is actually removed. |
Raised #6498 |
| @@ -48,7 +48,7 @@ chrono = { workspace = true } | |||
| chrono-tz = { version = "0.10", optional = true } | |||
| num = { version = "0.4.1", default-features = false, features = ["std"] } | |||
| half = { version = "2.1", default-features = false, features = ["num-traits"] } | |||
| hashbrown = { version = "0.14.2", default-features = false } | |||
| hashbrown = { version = "0.15.0", default-features = false, features = ["inline-more", "raw-entry"] } | |||
There was a problem hiding this comment.
Is setting inline-more strictly necessary? I think it's mutually exclusive from raw-entry as can compile without needing it 🤔
There was a problem hiding this comment.
I got that from reading the src. It's enabled by default for better performance, but we don't compile with the default features. I should have mentioned that in the PR description (sorry). This also points out how I should have really run the performance test. Thank you!
I'm also working on an alternative approach to remove use of raw-entry. I hope to performance test both approaches later today.
|
Before we merge this, we should certainly run benchmarks given how important hashbrown is to performance ALso there is a bunch of discussion downstream in DataFusion (which uses the lower level RawEntry APIs) on what to do now that hashbrown plans to remove it |
|
I also wanted to point out given how fundamental hashbrown is to a bunch of things (esp in DataFusion) I think we should let the new Thus I think we should target upgrading in the next arrow release (0.52.2) not the one I am preparing now |
Understood. I'll circle back to this one (or anyone else can feel free to take a shot), after I address some higher priority items. I'm partially into an alternative solution, but it's not near ready. I'll comment here when I pick it up again, if someone else doesn't beat me to it. 😆 |
|
FWIW I've filed an upstream ticket related to this, to see if the maintainers have any recommendations on how we should move forward here - rust-lang/hashbrown#573 |
|
With #6537 merged, I think we no longer need the |
|
Covered by #6684. |
Which issue does this PR close?
Alternative PR to #6486.
Rationale for this change
Update the ver 0.15 of hashbrown.
We are not using the deprecated
RawTable, only raw entries.In order to continue using raw entries, we need to use the inline-more and
raw-entryfeature flags.What changes are included in this PR?
See above.
Are there any user-facing changes?
No