-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore: remove phf from static files #10259
Conversation
thanks! i'd say we can just remove the whole thing, struct + trait, leaving only an optional field for backwards compatibility |
I have implemented a dummy Functions. Some function I don't familiar with , I just keep it. PTAL @joshieDo @shekhirin . Thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also remove this
reth/crates/storage/nippy-jar/src/lib.rs
Lines 396 to 400 in 97e153a
// Builds perfect hashing function from the values | |
if let Some(phf) = self.phf.as_mut() { | |
debug!(target: "nippy-jar", ?row_count, values_count = ?values.len(), "Setting keys for perfect hashing function."); | |
phf.set_keys(&values)?; | |
} |
set_keys
implemented anymore, and we can have just an empty enum or any other dummy field even without a hashmap.
remove whole struct and trait, PTAL. @joshieDo @shekhirin Thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there, also @joshieDo PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, LGTM! Waiting for @joshieDo review, and we will also need to check that this doesn't break existing clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ptal @joshieDo
looks good, just want to be sure and test it myself. either today or tomorrow 🫡 |
@joshieDo status on testing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, pushed test_config_serialization
. we can also remove the filter in a follow-up
would merge after release though
Wonder do some follow-up . |
I think its this place. reth/crates/static-file/types/src/filters.rs Lines 31 to 38 in 0b6480c
|
It seems that can merge. And I wonder do some follow-up. @shekhirin @joshieDo |
Closes #9946
use empty enum as dummy Functions