Skip to content

fix(compression): honor configured LZ4 compression #9879

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.

Sign up for GitHub

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

Open
dentiny wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(compression): honor configured LZ4 compression #9879

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8e5a84
fix(encoding): honor explicit LZ4 per-value compression
dentiny Oct 11, 2026
e1eecd4
docs(encoding): explain FSST per-value fallback
dentiny Oct 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions rust/lance-encoding/src/compression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,8 +808,10 @@ pub fn try_variable_width_per_value(
let data_size = data.expect_single_stat::<UInt64Type>(Stat::DataSize);
let per_value_requested = compression.is_some_and(|compression| compression != "fsst");
if (max_len > 32 * 1024 || per_value_requested) && data_size >= FSST_LEAST_INPUT_SIZE as u64 {
if compression == Some("zstd") {
let config = CompressionConfig::new(CompressionScheme::Zstd, params.compression_level);
// FSST uses a dedicated encoder and cannot be a general buffer compressor.
// Keep the existing default fallback when large values request FSST.
if let Some(compression) = compression.filter(|compression| *compression != "fsst") {
let config = CompressionConfig::new(compression.parse()?, params.compression_level);

@lance-gatekeeper lance-gatekeeper Bot Oct 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selecting LZ4 here makes nullable full-zip columns fail to read. Full-zip serialization omits the payload for nulls, and VariableFullZipDecoder::unzip reconstructs them as equal adjacent offsets. CompressedBufferEncoder::per_value_decompress then passes the empty slice to LZ4, which returns LZ4 compressed data too short; Zstd explicitly accepts empty slices. This affects ordinary scans of Utf8, LargeUtf8, Binary, and LargeBinary with nulls, across all three structural encodings.

Handle absent null payloads before invoking LZ4 and keep the existing end-to-end null coverage. Skipping empty slots in per-value decompression restored all 24 round-trip variants locally. The v13.0.0 reader has the same empty-input rejection, so a current-reader fix alone does not preserve stable-format readability: restrict or explicitly reject nullable full-zip LZ4 writes in stable formats until supported released readers can consume them.

Reproducer

In rust/lance-encoding/src/encodings/physical/block.rs, change the existing test_lz4_compress_round_trip expectation from .with_expected_encoding("zstd") to .with_expected_encoding("lz4"), then run:

cargo test -p lance-encoding test_lz4_compress_round_trip

Expected: round trips preserve values and nulls. Observed on this head: all 24 variants fail with LZ4 compressed data too short when the harness reaches null_rate=Some(0.5). The non-null cases pass. Restoring the pre-PR codec selector and original expectation makes all 24 variants pass.

return Ok(Some(Box::new(CompressedBufferEncoder::try_new(config)?)));
}
return Ok(Some(Box::new(CompressedBufferEncoder::default())));
Expand Down Expand Up @@ -1407,6 +1409,7 @@ mod tests {
use crate::buffer::LanceBuffer;
use crate::compression_config::CompressionParams;
use crate::data::{BlockInfo, DataBlock, FixedWidthDataBlock};
use crate::encodings::logical::primitive::fullzip::PerValueDataBlock;
use crate::encodings::logical::primitive::miniblock::MiniBlockCompressionContext;
use crate::statistics::ComputeStat;
use crate::testing::{TestEncoding, extract_array_encoding_chain, test_compression_strategy};
Expand Down Expand Up @@ -1994,6 +1997,42 @@ mod tests {
);
}

#[rstest::rstest]
#[cfg(feature = "lz4")]
#[case(32, 4096)]
#[case(64, 4096)]
#[case(32, 40 * 1024)]
#[case(64, 40 * 1024)]
fn test_per_value_honors_explicit_lz4(#[case] bits_per_offset: u8, #[case] value_size: usize) {
let mut field = create_test_field("body", DataType::Binary);
field
.metadata
.insert(COMPRESSION_META_KEY.to_string(), "lz4".to_string());
let strategy = baseline_strategy(CompressionParams::default());
let data = create_variable_width_block(bits_per_offset, 16, value_size);
let compressor = strategy.create_per_value(&field, &data).unwrap();
let (compressed, encoding) = compressor.compress(data.clone()).unwrap();
let Some(Compression::General(general)) = encoding.compression.as_ref() else {
panic!("Expected general per-value compression");
};
assert_eq!(
general.compression.as_ref().unwrap().scheme(),
crate::format::pb21::CompressionScheme::CompressionAlgorithmLz4
);

let PerValueDataBlock::Variable(compressed) = compressed else {
panic!("Expected variable-width compressed values");
};
let decoder = DefaultDecompressionStrategy::default()
.create_variable_per_value_decompressor(&encoding)
.unwrap();
let decoded = decoder.decompress(compressed).unwrap();
let expected = data.as_variable_width().unwrap();
let actual = decoded.as_variable_width().unwrap();
assert_eq!(actual.data.as_ref(), expected.data.as_ref());
assert_eq!(actual.offsets.as_ref(), expected.offsets.as_ref());
}

#[test]
fn test_parameter_merge_priority() {
let mut params = CompressionParams::new();
Expand Down
Loading