diff --git a/docs/src/format/file/versioning.md b/docs/src/format/file/versioning.md index 8024a46fbc8..a43be294070 100644 --- a/docs/src/format/file/versioning.md +++ b/docs/src/format/file/versioning.md @@ -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` 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. diff --git a/rust/lance-encoding/src/compression.rs b/rust/lance-encoding/src/compression.rs index 8b33de6c021..103e92e28ea 100644 --- a/rust/lance-encoding/src/compression.rs +++ b/rust/lance-encoding/src/compression.rs @@ -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, @@ -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)?, )), @@ -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 diff --git a/rust/lance-encoding/src/encodings/logical/primitive.rs b/rust/lance-encoding/src/encodings/logical/primitive.rs index 1c1a6eb404d..63a5a990d54 100644 --- a/rust/lance-encoding/src/encodings/logical/primitive.rs +++ b/rust/lance-encoding/src/encodings/logical/primitive.rs @@ -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, @@ -3497,6 +3496,24 @@ impl FixedFullZipDecoder { impl StructuralPageDecoder for FixedFullZipDecoder { fn drain(&mut self, num_rows: u64) -> Result> { + 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 { @@ -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::(); assert_eq!( FixedPerValueDecompressor::decoded_size_bytes(&decompressor, num_rows as u64), @@ -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), diff --git a/rust/lance-encoding/src/encodings/physical/value.rs b/rust/lance-encoding/src/encodings/physical/value.rs index 1757d15a9de..0dafa478f2d 100644 --- a/rust/lance-encoding/src/encodings/physical/value.rs +++ b/rust/lance-encoding/src/encodings/physical/value.rs @@ -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::{ @@ -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) } - _ => inner, - }; - let inner_encoding = match inner { DataBlock::FixedWidth(fixed_width) => { ProtobufUtils21::flat(fixed_width.bits_per_value, None) } @@ -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; @@ -528,7 +543,7 @@ impl ValueDecompressor { } } - pub fn from_fsl(mut description: &pb21::FixedSizeList) -> Self { + pub fn from_fsl(mut description: &pb21::FixedSizeList) -> Result { let mut layers = Vec::new(); let mut cum_dim = 1; let mut bytes_per_value = 0; @@ -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!(), } } } @@ -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() @@ -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(); @@ -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!( @@ -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, 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 @@ -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(); @@ -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!()