Skip to content

fix(encoding): prevent bits_per_value=0 in FullZipLayout for FSL with AllNull child #9130

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

Merged
westonpace merged 4 commits into from
Sep 11, 2026
Merged

fix(encoding): prevent bits_per_value=0 in FullZipLayout for FSL with AllNull child #9130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b7f0e01
fix(encoding): prevent bits_per_value=0 in FullZipLayout for FSL with…
westonpace Sep 10, 2026
b9ac6b4
fix(encoding): make legacy FSL AllNull pages readable with bits_per_v…
westonpace Sep 10, 2026
87e912a
fix(encoding): convert FSL unreachable! panics to proper errors and d…
westonpace Sep 11, 2026
b5bf84d
cargo fmt
westonpace Sep 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
29 changes: 29 additions & 0 deletions docs/src/format/file/versioning.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,32 @@ The following values are supported:
| legacy | N/A | N/A | Alias for 0.1 |
| stable | N/A | N/A | Alias for the default version for new datasets in the Lance release you are running. |
| next | N/A | N/A | Alias for the latest unstable version in the Lance release you are running.|

## Compatibility Caveats

Stable formats carry a compatibility guarantee, but certain data patterns exposed encoder bugs
that required encoding changes to fix. Files containing those patterns written by the fixed
encoder are not readable by readers predating the fix. The affected scenarios are listed here
so operators running mixed-version deployments know the minimum reader version required.

### FixedSizeList with all-null inner values (Lance 11.1.0)

**Affected format**: 2.1 and later.

**Scenario**: A `FixedSizeList` column where every inner value (not the outer list item itself)
is null — for example, `FixedSizeList<nullable Float32, dim=4>` where all eight Float32 values
across two outer rows are null.

**Buggy writer (Lance < 11.1.0)**: The encoder wrote `bits_per_value=0` into the FullZip page
layout. Readers of any version rejected these pages with an error, so the data was unreadable
regardless of reader version.

**Fixed writer (Lance ≥ 11.1.0)**: The encoder stores per-row validity bytes for the null inner
values, producing `bits_per_value > 0`. The fixed reader (Lance ≥ 11.1.0) can also decode the
old buggy pages, so old files written before 11.1.0 become readable after upgrading.

**Forward compatibility**: Files containing this pattern written by Lance ≥ 11.1.0 are **not
readable by Lance < 11.1.0**. Old readers encounter the `Compression::Constant` inner encoding
in the FSL descriptor and panic rather than returning an error.

**Minimum reader version for new files**: Lance 11.1.0.
6 changes: 3 additions & 3 deletions rust/lance-encoding/src/compression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,7 +1030,7 @@ impl DecompressionStrategy for DefaultDecompressionStrategy {
Compression::FixedSizeList(fsl) => {
// In the future, we might need to do something more complex here if FSL supports
// compression.
Ok(Box::new(ValueDecompressor::from_fsl(fsl)))
Ok(Box::new(ValueDecompressor::from_fsl(fsl)?))
}
Compression::Rle(rle) => Ok(Box::new(create_rle_decompressor(
rle,
Expand Down Expand Up @@ -1085,7 +1085,7 @@ impl DecompressionStrategy for DefaultDecompressionStrategy {
.map(|v| LanceBuffer::from_bytes(v.clone(), 1)),
))),
Compression::Flat(flat) => Ok(Box::new(ValueDecompressor::from_flat(flat))),
Compression::FixedSizeList(fsl) => Ok(Box::new(ValueDecompressor::from_fsl(fsl))),
Compression::FixedSizeList(fsl) => Ok(Box::new(ValueDecompressor::from_fsl(fsl)?)),
Compression::PackedStruct(description) => Ok(Box::new(
PackedStructFixedPerValueDecompressor::new(description)?,
)),
Expand Down Expand Up @@ -1181,7 +1181,7 @@ impl DecompressionStrategy for DefaultDecompressionStrategy {
}
Compression::Variable(_) => Ok(Box::new(BinaryBlockDecompressor::default())),
Compression::FixedSizeList(fsl) => {
Ok(Box::new(ValueDecompressor::from_fsl(fsl.as_ref())))
Ok(Box::new(ValueDecompressor::from_fsl(fsl.as_ref())?))
}
Compression::OutOfLineBitpacking(out_of_line) => {
// Extract the compressed bit width from the values encoding
Expand Down
31 changes: 24 additions & 7 deletions rust/lance-encoding/src/encodings/logical/primitive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3109,11 +3109,10 @@ impl FullZipScheduler {
let bytes_per_value = bits_per_value / 8;
let total_bytes_per_value =
bytes_per_value as usize + details.ctrl_word_parser.bytes_per_word();
if total_bytes_per_value == 0 {
return Err(lance_core::Error::internal(
"Invalid encoding: per-row byte width must be greater than 0",
));
}
// total_bytes_per_value == 0 is valid for constant-null FSL pages written by
// earlier encoders that produced bits_per_value=0 with no ctrl-word bytes.
// FixedFullZipDecoder::drain handles this case by producing AllNull output
// without touching the (empty) data buffer.
Ok(Box::new(FixedFullZipDecoder {
details,
data,
Expand Down Expand Up @@ -3497,6 +3496,24 @@ impl FixedFullZipDecoder {

impl StructuralPageDecoder for FixedFullZipDecoder {
fn drain(&mut self, num_rows: u64) -> Result<Box<dyn DecodePageTask>> {
if self.total_bytes_per_value == 0 {
// No bytes per row: constant-null page with no ctrl-word bytes.
// The decompressor (ConstantDecompressor) ignores its input and returns AllNull.
return Ok(Box::new(FixedFullZipDecodeTask {
details: self.details.clone(),
data: vec![FullZipDecodeTaskItem {
data: PerValueDataBlock::Fixed(FixedWidthDataBlock {
data: LanceBuffer::empty(),
bits_per_value: 0,
num_values: num_rows,
block_info: BlockInfo::new(),
}),
rows_in_buf: num_rows,
}],
bytes_per_value: 0,
num_rows: num_rows as usize,
}));
}
let mut task_data = Vec::with_capacity(self.data.len());
let mut remaining = num_rows;
while remaining > 0 {
Expand Down Expand Up @@ -7547,7 +7564,7 @@ mod tests {
let Compression::FixedSizeList(fsl) = compression.compression.unwrap() else {
panic!("expected fixed-size-list compression");
};
let decompressor = ValueDecompressor::from_fsl(fsl.as_ref());
let decompressor = ValueDecompressor::from_fsl(fsl.as_ref()).unwrap();
let expected_size = num_rows * dimension * size_of::<f32>();
assert_eq!(
FixedPerValueDecompressor::decoded_size_bytes(&decompressor, num_rows as u64),
Expand Down Expand Up @@ -7619,7 +7636,7 @@ mod tests {
panic!("expected fixed-size-list compression");
};
let decompressor = NullableFslDecompressor {
inner: ValueDecompressor::from_fsl(fsl.as_ref()),
inner: ValueDecompressor::from_fsl(fsl.as_ref()).unwrap(),
};
assert_eq!(
FixedPerValueDecompressor::decoded_size_bytes(&decompressor, num_rows as u64),
Expand Down
116 changes: 88 additions & 28 deletions rust/lance-encoding/src/encodings/physical/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ use crate::compression::{
require_block_payload,
};
use crate::data::{
BlockInfo, DataBlock, FixedSizeListBlock, FixedWidthDataBlock, NullableDataBlock,
AllNullDataBlock, BlockInfo, DataBlock, FixedSizeListBlock, FixedWidthDataBlock,
NullableDataBlock,
};
use crate::encodings::logical::primitive::fullzip::{PerValueCompressor, PerValueDataBlock};
use crate::encodings::logical::primitive::miniblock::{
Expand Down Expand Up @@ -322,17 +323,18 @@ impl ValueEncoder {
fn fsl_to_encoding(fsl: &FixedSizeListBlock) -> CompressiveEncoding {
let mut inner = fsl.child.as_ref();
let mut has_validity = false;
inner = match inner {
DataBlock::Nullable(nullable) => {
has_validity = true;
nullable.data.as_ref()
}
if let DataBlock::Nullable(nullable) = inner {
has_validity = true;
inner = nullable.data.as_ref();
}
let inner_encoding = match inner {
// All inner values are null. Reserve validity bits (has_validity=true) so that
// the decoder knows one validity byte per cum_dim items is stored per row.
// constant(None) signals that every stored item decodes to null.
DataBlock::AllNull(_) => {
return ProtobufUtils21::constant(None);
has_validity = true;
ProtobufUtils21::constant(None)

@lance-gatekeeper lance-gatekeeper Bot Sep 10, 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.

This makes stable v2.1 writers emit FixedSizeList(has_validity=true) → Constant(None). Lance v11.0.0 is the latest stable release and resolves stable to v2.1, but its ValueDecompressor::from_fsl accepts only nested FixedSizeList and a terminal Flat; this terminal reaches _ => unreachable!(). Files written through this new path therefore panic in that released reader. The same-version round-trip test cannot detect this forward-compatibility break. Please keep stable writes on an encoding shape v11 understands (for example, a Flat terminal with masked payload bytes at the actual leaf width), or version-gate this composition to an unstable format.

Reproducer against v11.0.0 
#[test]
fn accepts_nested_constant_fsl_value_encoding() {
    let encoding = super::ProtobufUtils21::fsl(
        4,
        true,
        super::ProtobufUtils21::constant(None),
    );
    let Compression::FixedSizeList(fsl) = encoding.compression.unwrap() else {
        panic!("expected fixed-size-list encoding");
    };

    let _ = ValueDecompressor::from_fsl(fsl.as_ref());
}

cargo test -p lance-encoding accepts_nested_constant_fsl_value_encoding -- --nocapture

Expected: the released stable reader accepts the stable v2.1 encoding. Observed: internal error: entered unreachable code in ValueDecompressor::from_fsl.

@westonpace westonpace Sep 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The old writer would have crashed without this change. We are not making things worse in terms of compatibility.

@lance-gatekeeper lance-gatekeeper Bot Sep 10, 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.

The old writer failure is the bug being fixed, but it does not make the new stable encoding forward-compatible: this patch makes a v2.1 file readable by the current reader while the released stable v11 reader still panics on its nested Constant terminal. The repository contract requires every new stable-format write to remain readable by the released stable reader, even when the old writer could not successfully produce that edge case, so this finding remains.

@westonpace westonpace Sep 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I accept this as a known risk. Masked payload bytes would be inefficient and this is a fairly niche corner case already.

@westonpace westonpace Sep 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually, I guess I'll try and fix it.

@lance-gatekeeper lance-gatekeeper Bot Sep 10, 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.

The follow-up commit only permits zero-width pages in the current FullZip scheduler; it does not change fsl_to_encoding, which still emits stable v2.1 FixedSizeList → Constant. The released v11 reader therefore still reaches its unsupported terminal, so the compatibility finding remains.

@westonpace westonpace Sep 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct. I am accepting (and have documented) this compatibility gap. The old writer created files that failed anyways. The new writer creates files which panic instead of error, which is unfortunate. However, the compatibility path could lead to 1000x+ increase in storage cost for these types and that is unacceptable. A break in compatibility is the best path forward for this niche case.

@lance-gatekeeper lance-gatekeeper Bot Sep 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.

The storage-cost concern rules out the padded stable encoding, but not the other cited path: put this composition behind an unstable format version. Stable-format forward compatibility is an explicit repository contract, so documenting a v2.1 exception cannot waive it; this finding remains.

@lance-gatekeeper lance-gatekeeper Bot Sep 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.

The current head contains only a formatting change and does not alter this assessment. Version-gating the new composition avoids the padded storage cost while preserving the explicit stable-format compatibility contract, so the finding remains.

}
_ => inner,
};
let inner_encoding = match inner {
DataBlock::FixedWidth(fixed_width) => {
ProtobufUtils21::flat(fixed_width.bits_per_value, None)
}
Expand Down Expand Up @@ -411,6 +413,19 @@ impl ValueEncoder {
break;
}
DataBlock::AllNull(_) => {
// All inner values are null. Add all-zero validity bits so that
// bytes_per_row > 0 and the FullZip layout doesn't write bits_per_value=0,
// which would crash the reader when there are also no ctrl-word bytes.
bytes_per_row += cum_dim.div_ceil(8) as usize;
validity_iters.push(PerValueFslValidityIter {
buffer: LanceBuffer::from(vec![
0u8;
cum_dim.div_ceil(8) as usize
* num_values as usize
]),
bits_per_row: cum_dim as usize,
offset: 0,
});
data_bytes_per_row = 0;
data_buffer = LanceBuffer::empty();
break;
Expand Down Expand Up @@ -528,7 +543,7 @@ impl ValueDecompressor {
}
}

pub fn from_fsl(mut description: &pb21::FixedSizeList) -> Self {
pub fn from_fsl(mut description: &pb21::FixedSizeList) -> Result<Self> {
let mut layers = Vec::new();
let mut cum_dim = 1;
let mut bytes_per_value = 0;
Expand All @@ -541,28 +556,45 @@ impl ValueDecompressor {
if description.has_validity {
bytes_per_value += cum_dim.div_ceil(8);
}
match description
let encoding = description
.values
.as_ref()
.unwrap()
.ok_or_else(|| Error::invalid_input("FSL encoding missing inner values field"))?
.compression
.as_ref()
.unwrap()
{
.ok_or_else(|| {
Error::invalid_input("FSL encoding missing inner compression field")
})?;
match encoding {
Compression::FixedSizeList(inner) => {
description = inner;
}
Compression::Flat(flat) => {
let mut bits_per_value = bytes_per_value * 8;
bits_per_value += flat.bits_per_value * cum_dim;
return Self {
return Ok(Self {
bits_per_item: flat.bits_per_value,
bits_per_value,
items_per_value: cum_dim,
layers,
};
});
}
// All inner values are null: only validity bytes are stored per row.
// bits_per_item=0 signals unzip_decompress to emit AllNull for the values.
Compression::Constant(_) => {
return Ok(Self {
bits_per_item: 0,
bits_per_value: bytes_per_value * 8,
items_per_value: cum_dim,
layers,
});
}
_ => {
return Err(Error::invalid_input(format!(
"Unexpected inner encoding type in FSL descriptor: {:?}",
encoding
)));
}
_ => unreachable!(),
}
}
}
Expand Down Expand Up @@ -707,12 +739,23 @@ impl ValueDecompressor {
}

// Finally, restore the structure
let mut block = DataBlock::FixedWidth(FixedWidthDataBlock {
bits_per_value: self.bits_per_item,
num_values: num_items as u64,
data: LanceBuffer::from(data_buffer),
block_info: BlockInfo::new(),
});
//
// bits_per_item=0 means the terminal encoding is constant-null (all inner values
// are null and only validity bits were stored). Use AllNull so that into_arrow
// produces a properly typed all-null array without a zero-width data buffer.
let mut block = if self.bits_per_item == 0 {
debug_assert!(data_buffer.is_empty());
DataBlock::AllNull(AllNullDataBlock {
num_values: num_items as u64,
})
} else {
DataBlock::FixedWidth(FixedWidthDataBlock {
bits_per_value: self.bits_per_item,
num_values: num_items as u64,
data: LanceBuffer::from(data_buffer),
block_info: BlockInfo::new(),
})
};

let mut validity_bufs = buffer_builders
.into_iter()
Expand Down Expand Up @@ -1032,7 +1075,7 @@ mod tests {
panic!()
};

let decompressor = ValueDecompressor::from_fsl(fsl.as_ref());
let decompressor = ValueDecompressor::from_fsl(fsl.as_ref()).unwrap();

let decompressed =
MiniBlockDecompressor::decompress(&decompressor, data.data, data.num_values).unwrap();
Expand Down Expand Up @@ -1120,7 +1163,7 @@ mod tests {
panic!()
};

let decompressor = ValueDecompressor::from_fsl(fsl.as_ref());
let decompressor = ValueDecompressor::from_fsl(fsl.as_ref()).unwrap();

let num_values = data.num_values;
assert_eq!(
Expand Down Expand Up @@ -1154,6 +1197,23 @@ mod tests {
.await;
}

// Regression: nullable_per_value_fsl wrote bits_per_value=0 when the child block was
// AllNull but the FSL had no outer null buffer (no ctrl-word bytes), making
// total_bytes_per_value=0 and crashing the reader with "per-row byte width must be > 0".
#[test_log::test(tokio::test)]
async fn test_fsl_nullable_child_all_null_no_outer_nulls() {
// FSL<nullable Float32, dim=4>, 2 outer rows, no outer null buffer, all 8 child
// Float32 values are null. DataBlock::from_arrays returns AllNull for the child,
// which routes into nullable_per_value_fsl. Without this fix the encoder writes
// bits_per_value=0 and the decoder errors on read.
let items = new_null_array(&DataType::Float32, 8);
let items_field = Arc::new(Field::new("item", DataType::Float32, true));
let fsl = FixedSizeListArray::new(items_field, 4, items, None);

let test_cases = TestCases::default().with_structural_encodings();
check_round_trip_encoding_of_data(vec![Arc::new(fsl)], &test_cases, HashMap::new()).await;
}

#[test_log::test(tokio::test)]
async fn regress_list_fsl() {
// This regresses a case where rows are large lists that span multiple
Expand Down Expand Up @@ -1205,7 +1265,7 @@ mod tests {
panic!()
};

let decompressor = ValueDecompressor::from_fsl(fsl.as_ref());
let decompressor = ValueDecompressor::from_fsl(fsl.as_ref()).unwrap();

let decompressed =
MiniBlockDecompressor::decompress(&decompressor, data.data, data.num_values).unwrap();
Expand Down Expand Up @@ -1235,7 +1295,7 @@ mod tests {
panic!()
};

let decompressor = ValueDecompressor::from_fsl(fsl.as_ref());
let decompressor = ValueDecompressor::from_fsl(fsl.as_ref()).unwrap();

let PerValueDataBlock::Fixed(data) = data else {
panic!()
Expand Down
Loading