Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Shouldn't the place where the original bytes are written be changed from native endian to little endian instead? This change asserts that the serialization format is endianness dependent, when I think it should use a fixed endianness.
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.
As far as I can tell with some smaller tests this does not change how the hash table is serialized. That said there may be a better solution, as far as I can tell this issues comes from some assumptions of layout of arrays in memory. The most simple test that is failing is probably this one https://github.com/rust-lang/odht/blob/main/src/swisstable_group_query/mod.rs#L58..L73
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.
I believe the underlying format is byte oriented, so endianness doesn't matter in the serialization format.
However, this
GroupWord
is batching theu8
control words to try to efficiently scan for matches or empties. It's using the bit-orientedtrailing_zeros()
to produce byte-orientedusize
indexes into the control words. It makes sense that we would wantfrom_le_bytes
for that, to ensure low bytes are loaded at the correct "trailing" end. So I don't really understand what's happening here, whyfrom_ne_bytes
is fixing anything.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.
Aha! The values
eq_mask
andempty_mask
also have ato_le()
on their construction, so they're effectively flipped twice. We can either flip withfrom_le_bytes(..)
or theseto_le()
, but should not do both.