fix(rust): `VariableByte` not matching the cpp version by CommanderStorm · Pull Request #60 · fast-pack/FastPFOR-rs

CommanderStorm · 2026-02-14T18:23:22Z

VariableByte currently matches none of the schemes in the Cpp impl.
Now it matches MaskedVByteCodec or VByteCodec.

I don't quite know why VB is implemented differently compared to "the reference".
The codec appears in 6e232dd#diff-01152c24d242e2ecf9520ee8a8bbb63b7aa622e3d4a530a19f4acfe9f970f7ad

I am not entirely sure if it is 100% correct, but it is better than before (as in: actually passes tests).

I am sure that there are 2-3 correctness issues in this code.. grumble..
I am also not sure about some of the chocies in Cpp, for example why 10? .. 🤷🏻‍♂️

codecov · 2026-02-14T18:27:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

This PR fixes the Rust VariableByte codec implementation to match the C++ MaskedVByteCodec behavior, enabling proper interoperability between Rust and C++ implementations. The previous implementation did not correctly encode/decode variable-byte integers, which is now fixed along with comprehensive test coverage.

Changes:

Fixed variable-byte encoding/decoding logic for both u32 and i8 output formats
Moved helper functions from shared module to codec-specific const fn methods with const generics
Updated fuzzing targets to use MaskedVByteCodec instead of VByteCodec for oracle testing
Added comprehensive test coverage with 18 new test cases covering edge cases and boundary values
Fixed test harness to properly track compressed data length using output_offset

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/rust/integer_compression/variable_byte.rs	Complete rewrite of compression/decompression logic; added helper methods and 18 new comprehensive tests
src/rust/integer_compression/helpers.rs	Removed codec-specific extract7bits functions (moved to VariableByte)
tests/basic_tests.rs	Fixed compressed length tracking using output_offset position
fuzz/fuzz_targets/rust_decompress_oracle.rs	Switched from VByteCodec to MaskedVByteCodec; removed VariableByte skip condition
fuzz/fuzz_targets/rust_compress_oracle.rs	Switched from VByteCodec to MaskedVByteCodec; removed VariableByte skip condition; improved variable naming
fuzz/fuzz_targets/common.rs	Improved debug formatting with HexSlice helper for better fuzzing diagnostics

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-14T18:28:38Z

+        let input_bytes: &[u8] = unsafe {
+            std::slice::from_raw_parts(input[input_start..].as_ptr().cast::<u8>(), byte_length)
+        };
+
+        let mut byte_pos = 0;
+        let mut tmp_outpos = output_offset.position() as usize;
+
+        // Fast path: process while we have at least 10 bytes remaining
+        while byte_pos + 10 <= byte_length {
+            let mut v: u32 = 0;
+            let mut bytes_read = 0;
+
+            // Decode up to 5 bytes for a u32 value
+            for i in 0..5 {
+                let c = input_bytes[byte_pos + i];
+
+                if i < 4 {
+                    // For bytes 0-3, use 7 bits each
+                    v |= u32::from(c & 0x7F) << (i * 7);
+                    if c < 128 {
+                        bytes_read = i + 1;
+                        break;
+                    }
+                } else {
+                    // For byte 4, use only 4 bits (total: 7*4 + 4 = 32 bits)
+                    v |= u32::from(c & 0x0F) << 28;
+                    bytes_read = 5;
+                }
+            }
+
+            byte_pos += bytes_read;
+            output[tmp_outpos] = v;
+            tmp_outpos += 1;


Similar to the compress function, this unsafe code doesn't validate buffer sizes. The code should verify that enough space exists in the output buffer before writing. Add a check like: if output.len() <= tmp_outpos { return Err(FastPForError::NotEnoughSpace); } before each write to output[tmp_outpos]. Alternatively, check at the start that output has sufficient capacity for the expected number of integers.

Copilot · 2026-02-14T18:28:38Z

+        while byte_pos < byte_length {
+            let mut shift = 0;
+            let mut v: u32 = 0;
+            while byte_pos < byte_length {
+                let c = input_bytes[byte_pos];
+                byte_pos += 1;
+                v += u32::from(c & 127) << shift;
+                if c < 128 {
+                    output[tmp_outpos] = v;
+                    tmp_outpos += 1;
+                    break;
+                }
                shift += 7;
            }


The inner while loop (lines 188-198) doesn't have a safety check to prevent infinite loops if the data is malformed. If all bytes have the high bit set (c >= 128) and we reach the end of input_bytes without finding a terminating byte (c < 128), the loop will exit but leave an incomplete value. Additionally, the shift value can grow unbounded (shift += 7 on line 197). Add a check like: if shift >= 32 { return Err(FastPForError::InvalidData); } to prevent malformed data from causing issues.

Copilot · 2026-02-14T18:28:39Z

        while p < final_p {
-            v = i32::from(input[p as usize]);
-            if input[p as usize] < 0 {
+            let mut v = i32::from(input[p as usize] & 0x7F);
+            if input[p as usize] >= 0 {
+                // High bit is NOT set, this is the last byte
                p += 1;
+                output[tmp_outpos as usize] = v as u32;
+                tmp_outpos += 1;
                continue;
            }
-            v |= i32::from(input[p as usize + 1]) << 7;
-            if input[p as usize + 1] < 0 {
+
+            v |= i32::from(input[p as usize + 1] & 0x7F) << 7;
+            if input[p as usize + 1] >= 0 {
+                // High bit is NOT set, this is the last byte
                p += 2;
+                output[tmp_outpos as usize] = v as u32;
+                tmp_outpos += 1;
                continue;
            }
-            v |= i32::from(input[p as usize + 2]) << 14;
-            if input[p as usize + 2] < 0 {
+
+            v |= i32::from(input[p as usize + 2] & 0x7F) << 14;
+            if input[p as usize + 2] >= 0 {
+                // High bit is NOT set, this is the last byte
                p += 3;
+                output[tmp_outpos as usize] = v as u32;
+                tmp_outpos += 1;
                continue;
            }
-            v |= i32::from(input[p as usize + 3]) << 21;
-            if input[p as usize + 3] < 0 {
+
+            v |= i32::from(input[p as usize + 3] & 0x7F) << 21;
+            if input[p as usize + 3] >= 0 {
+                // High bit is NOT set, this is the last byte
                p += 4;
+                output[tmp_outpos as usize] = v as u32;
+                tmp_outpos += 1;
                continue;
            }
-            v |= i32::from(input[p as usize + 4]) << 28;
+
+            v |= i32::from(input[p as usize + 4] & 0x0F) << 28;


The decompression logic has potential out-of-bounds reads. When checking input[p as usize + N] (lines 287, 296, 305), there's no verification that p + N < final_p. If the input data is malformed or truncated, this could read beyond the valid input range. Add bounds checks before each read, for example: if p + N >= final_p { return Err(FastPForError::NotEnoughSpace); }

Copilot · 2026-02-14T18:28:39Z

+                output_bytes[byte_pos] = Self::extract_7bits::<0>(val) | (1 << 7);
+                output_bytes[byte_pos + 1] = Self::extract_7bits::<1>(val) | (1 << 7);
+                output_bytes[byte_pos + 2] = Self::extract_7bits::<2>(val) | (1 << 7);
+                output_bytes[byte_pos + 3] = Self::extract_7bits::<3>(val) | (1 << 7);
+                output_bytes[byte_pos + 4] = Self::extract_7bits_maskless::<4>(val);
+                byte_pos += 5;


There's a mismatch between compression and decompression for the 5th byte. In compression (line 89), the 5th byte stores extract_7bits_maskless which extracts 7 bits (bits 28-34 of the value). However, in decompression (line 174), only 4 bits are extracted from the 5th byte using mask 0x0F. This means values using 5 bytes cannot roundtrip correctly.

For a u32 value, we have 32 bits total. With variable byte encoding using 7 bits per byte, we can encode:

Bytes 0-3: 7 bits each = 28 bits total

Byte 4: remaining 4 bits (32 - 28 = 4)

The compression should extract only 4 bits for the 5th byte, not 7 bits. Either use extract_7bits with a 0x0F mask, or create a helper that extracts 4 bits.

Copilot · 2026-02-14T18:28:39Z

+                output[out_pos_tmp as usize] = (Self::extract_7bits::<3>(val) | (1 << 7)) as i8;
                out_pos_tmp += 1;
-                output[out_pos_tmp as usize] = (extract_7bits_maskless(4, val) | (1 << 7)) as i8;
+                output[out_pos_tmp as usize] = Self::extract_7bits_maskless::<4>(val) as i8;


Same issue as in the u32 compress function: the 5th byte stores 7 bits (extract_7bits_maskless) instead of only 4 bits. This is inconsistent with the decompression logic at line 314 which only extracts 4 bits (0x0F mask) from the 5th byte. For correct roundtrip encoding/decoding of u32 values, the 5th byte should only store the top 4 bits (bits 28-31), not 7 bits.

Suggested change

output[out_pos_tmp as usize] = Self::extract_7bits_maskless::<4>(val) as i8;

output[out_pos_tmp as usize] = ((val >> 28) & 0x0F) as i8;

Copilot · 2026-02-14T18:28:39Z

        .expect("Failed to compress");
-        let mut answer: Vec<u32> = vec![];
+
+        let encoded_len = (output_offset.position() - input.len() as u64) as u32;


This calculation appears to be a workaround for a pre-existing bug in the i8 compress function at line 261 (outside this PR's scope), where output_offset is incorrectly incremented by input_length. However, rather than working around the bug in tests, the bug should be fixed in the compress function itself. The compress function should set output_offset.set_position(out_pos_tmp) without adding input_length, and then this test line should use output_offset.position() as u32 directly.

Suggested change

let encoded_len = (output_offset.position() - input.len() as u64) as u32;

let encoded_len = output_offset.position() as u32;

Copilot · 2026-02-14T18:28:39Z

+        let output_bytes: &mut [u8] = unsafe {
+            std::slice::from_raw_parts_mut(
+                output[output_start..].as_mut_ptr().cast::<u8>(),
+                (output.len() - output_start) * 4,


The unsafe code creates a byte slice without validating that the output buffer has sufficient capacity. Before calling from_raw_parts_mut, there should be a check to ensure that (output.len() - output_start) * 4 bytes are sufficient for the maximum possible compressed size. In the worst case, each u32 value could require 5 bytes plus padding. Consider adding: let max_bytes_needed = (input_length as usize * 5 + 3) & !3; if (output.len() - output_start) * 4 < max_bytes_needed { return Err(...); }

Suggested change

let output_bytes: &mut [u8] = unsafe {

std::slice::from_raw_parts_mut(

output[output_start..].as_mut_ptr().cast::<u8>(),

(output.len() - output_start) * 4,

let available_bytes = (output.len() - output_start) * 4;

// Worst-case: each u32 may require up to 5 bytes, then pad to 4-byte alignment

let max_bytes_needed = ((input_length as usize * 5) + 3) & !3;

if available_bytes < max_bytes_needed {

return Err(FastPForError::InsufficientOutputBuffer);

}

let output_bytes: &mut [u8] = unsafe {

std::slice::from_raw_parts_mut(

output[output_start..].as_mut_ptr().cast::<u8>(),

available_bytes,

CommanderStorm added 8 commits February 14, 2026 17:32

make the fuzzing-formatter more pleasant

a2225c9

fix varbyte decoding

17d59f7

fix testcase

d11517b

update fuzzer

07e1da2

remove tests

3378be5

simplify

1ffa3a3

simplify the code

454e509

simplify the compressor

fcc859f

CommanderStorm requested review from Copilot, jjcfrancisco and nyurik February 14, 2026 18:23

Copilot started reviewing on behalf of CommanderStorm February 14, 2026 18:23 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

remove inlining instructions

62ce834

nyurik approved these changes Feb 14, 2026

View reviewed changes

CommanderStorm merged commit 14687bf into fast-pack:main Feb 15, 2026
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(rust): `VariableByte` not matching the cpp version#60

fix(rust): `VariableByte` not matching the cpp version#60
CommanderStorm merged 9 commits into
fast-pack:mainfrom
CommanderStorm:fix-varbyte

CommanderStorm commented Feb 14, 2026

Uh oh!

codecov Bot commented Feb 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	output[out_pos_tmp as usize] = Self::extract_7bits_maskless::<4>(val) as i8;
	output[out_pos_tmp as usize] = ((val >> 28) & 0x0F) as i8;

	let encoded_len = (output_offset.position() - input.len() as u64) as u32;
	let encoded_len = output_offset.position() as u32;

Uh oh!

Conversation

CommanderStorm commented Feb 14, 2026

Uh oh!

codecov Bot commented Feb 14, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants