-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: Incorrect inlined string view comparison after Add prefix compar… #7875
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
Changes from 5 commits
3dff4c1
f5fb49a
02c1e95
3884cbb
1abc59c
35b0f4b
8cf8949
d3966b1
3d2765a
07de90f
2c2df16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -605,29 +605,33 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> { | |
| /// key("bar\0") = 0x0000000000000000000062617200000004 | ||
| /// ⇒ key("bar") < key("bar\0") | ||
| /// ``` | ||
| /// | ||
| /// ### Why the old code failed | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be more helpful to future readers to explain here to explain how the calculation works, rather than explaining why a previous attempt didnt' work 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it @alamb , thank you! |
||
| /// Fixed in: <https://github.com/apache/arrow-rs/pull/7875> | ||
| /// In the previous implementation, we did: | ||
| /// ```ignore | ||
| /// let word_be = u128::from_le_bytes(raw.to_le_bytes()).to_be(); | ||
| /// ``` | ||
| /// That single `.to_be()` flips **all 16 bytes**, including the data region, effectively | ||
| /// reversing each string. As a result: | ||
| /// - `"backend one"` was compared as if it were `"eno dnekcab"`, inverting lexicographical order | ||
| /// - any multi‑byte string ordering was completely incorrect | ||
| #[inline(always)] | ||
| pub fn inline_key_fast(raw: u128) -> u128 { | ||
| // Convert the raw u128 (little-endian) into bytes for manipulation | ||
| let raw_bytes = raw.to_le_bytes(); | ||
|
|
||
| // Extract the length (first 4 bytes), convert to big-endian u32, and promote to u128 | ||
| let len_le = &raw_bytes[0..4]; | ||
| let len_be = u32::from_le_bytes(len_le.try_into().unwrap()).to_be() as u128; | ||
|
|
||
| // Extract the inline string bytes (next 12 bytes), place them into the lower 12 bytes of a 16-byte array, | ||
| // padding the upper 4 bytes with zero to form a little-endian u128 value | ||
| let mut inline_bytes = [0u8; 16]; | ||
| inline_bytes[4..16].copy_from_slice(&raw_bytes[4..16]); | ||
|
|
||
| // Convert to big-endian to ensure correct lexical ordering | ||
| let inline_u128 = u128::from_le_bytes(inline_bytes).to_be(); | ||
| // Parse native length from LE, then write BE bytes | ||
| let length = u32::from_le_bytes(raw_bytes[0..4].try_into().unwrap()); | ||
|
zhuqi-lucas marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Shift right by 32 bits to discard the zero padding (upper 4 bytes), | ||
| // so that the inline string occupies the high 96 bits | ||
| let inline_part = inline_u128 >> 32; | ||
| // Build a 16-byte buffer with: | ||
| // - bytes [0..12] = inline string data | ||
| // - bytes [12..16] = big-endian length | ||
| let mut buf = [0u8; 16]; | ||
| buf[0..12].copy_from_slice(&raw_bytes[4..16]); | ||
| buf[12..16].copy_from_slice(&length.to_be_bytes()); | ||
|
|
||
| // Combine the inline string part (high 96 bits) and length (low 32 bits) into the final key | ||
| (inline_part << 32) | len_be | ||
| // Interpret as a big-endian u128 for final comparison key | ||
| u128::from_be_bytes(buf) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1193,9 +1197,19 @@ mod tests { | |
| b"abcdefghi", | ||
| b"abcdefghij", | ||
| b"abcdefghijk", | ||
| b"abcdefghijkl", // 12 bytes, max inline | ||
| b"bar", | ||
| b"bar\0", // special case to test length tiebreaker | ||
| b"abcdefghijkl", | ||
| // ─────────────────────────────────────────────────────────────────────── | ||
| // This pair verifies that we didn’t accidentally reverse the inline bytes: | ||
| // without our fix, “backend one” would compare as if it were | ||
| // “eno dnekcab”, so “one” might end up sorting _after_ “two”. | ||
| b"backend one", // special case: tests byte-order reversal bug | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why this wasn't this caught withthe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, because : The bug caused full byte reversal of the inline string bytes, meaning the entire 12-byte segment was reversed before comparison. For strings like "xyy" and "xyz", which differ only in their last byte, reversing the bytes moves this difference to the first byte of the reversed string. Since comparisons are done on the reversed bytes for both strings, the order is consistently flipped but preserved between them. Thus, even though the byte order is wrong globally (the entire string is reversed), "xyy" still compares correctly as less than "xyz" in the reversed space, so the test passes. In other words, differences at the end of short strings don’t expose the reversal bug, because reversing the entire string simply moves the difference to the front, preserving the relative order. The bug only becomes apparent in strings with differences in the middle or earlier bytes, like "backend one" vs "backend two", where reversing the entire inline data inverts the lexicographical order unexpectedly.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess because
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right! |
||
| b"backend two", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also please include strings that have the same exact inline prefix the length differ as well as content ? Something like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, given the endian swapping going on, can we please also include a few strings that are more than 256 bytes long (so the length requires 2 bytes to store)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @alamb for good point: b"than12Byt",
b"than12Bytes",
b"than12Bytes\0",
b"than12Bytez",I will add above cases, because inline_key_fast function only used for <= 12 and <= 12 bytes to compare.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in latest PR for above testing case.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @alamb I also found a way to do it, i adjust the fuzz testing to reproduce the cases, and this PR will make the fuzz testing works well. |
||
| // ─────────────────────────────────────────────────────────────────────── | ||
| // This pair exercises the length‐tiebreaker logic: | ||
| // both “bar” (3 bytes) and “bar\0” (4 bytes) yield identical 96‑bit data | ||
| // (62 61 72 00…00), so only the 32‑bit length field can break the tie. | ||
| b"bar", // special case: ensures shorter string sorts first | ||
| b"bar\0", // special case: null‑padded variant tests length tiebreaker | ||
| b"xyy", | ||
| b"xyz", | ||
| ]; | ||
|
|
||
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 think this diagram needs be updated as well