Skip to content

Commit af8a90f

Browse files
zhangyue19921010zhangyue19921010
andauthored
fix: return error instead of panicking on values too wide for miniblock (#7650)
Return error instead of panicking on values too wide for miniblock <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of oversized values during miniblock encoding by returning a clear `InvalidInput` error instead of failing unexpectedly. * Fixed fixed-width and fixed-size-list miniblock encoding paths to consistently propagate invalid input failures. * Expanded test coverage for wide binary and list inputs (including boolean lists) to verify the error type and that the message includes both the “too wide” detail and the expected value requirement. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: zhangyue19921010 <zhangyue.1010@bytedance.com>
1 parent a33cd39 commit af8a90f

1 file changed

Lines changed: 74 additions & 15 deletions

File tree

  • rust/lance-encoding/src/encodings/physical

rust/lance-encoding/src/encodings/physical/value.rs

Lines changed: 74 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,27 +27,33 @@ pub struct ValueEncoder {}
2727

2828
impl ValueEncoder {
2929
/// Use the largest chunk we can smaller than 4KiB
30-
fn find_log_vals_per_chunk(bytes_per_word: u64, values_per_word: u64) -> (u64, u64) {
30+
fn find_log_vals_per_chunk(bytes_per_word: u64, values_per_word: u64) -> Result<(u64, u64)> {
3131
let mut size_bytes = 2 * bytes_per_word;
3232
let (mut log_num_vals, mut num_vals) = match values_per_word {
3333
1 => (1, 2),
3434
8 => (3, 8),
3535
_ => unreachable!(),
3636
};
3737

38-
// If the type is so wide that we can't even fit 2 values we shouldn't be here
39-
assert!(size_bytes < MAX_MINIBLOCK_BYTES);
38+
if size_bytes >= MAX_MINIBLOCK_BYTES {
39+
let num_values = 2 * values_per_word;
40+
return Err(Error::invalid_input(format!(
41+
"Value is too wide for miniblock encoding: {} values require {} bytes but a \
42+
miniblock chunk is limited to {} bytes.",
43+
num_values, size_bytes, MAX_MINIBLOCK_BYTES
44+
)));
45+
}
4046

4147
while 2 * size_bytes < MAX_MINIBLOCK_BYTES && 2 * num_vals <= *MAX_MINIBLOCK_VALUES {
4248
log_num_vals += 1;
4349
size_bytes *= 2;
4450
num_vals *= 2;
4551
}
4652

47-
(log_num_vals, num_vals)
53+
Ok((log_num_vals, num_vals))
4854
}
4955

50-
fn chunk_data(data: FixedWidthDataBlock) -> MiniBlockCompressed {
56+
fn chunk_data(data: FixedWidthDataBlock) -> Result<MiniBlockCompressed> {
5157
// Usually there are X bytes per value. However, when working with boolean
5258
// or FSL<boolean> we might have some number of bits per value that isn't
5359
// divisible by 8. In this case, to avoid chunking in the middle of a byte
@@ -60,7 +66,7 @@ impl ValueEncoder {
6066

6167
// Aim for 4KiB chunks
6268
let (log_vals_per_chunk, vals_per_chunk) =
63-
Self::find_log_vals_per_chunk(bytes_per_word, values_per_word);
69+
Self::find_log_vals_per_chunk(bytes_per_word, values_per_word)?;
6470
let num_chunks = bit_util::ceil(data.num_values as usize, vals_per_chunk as usize);
6571
debug_assert_eq!(vals_per_chunk % values_per_word, 0);
6672
let bytes_per_chunk = bytes_per_word * (vals_per_chunk / values_per_word);
@@ -99,11 +105,11 @@ impl ValueEncoder {
99105

100106
debug_assert_eq!(chunks.len(), num_chunks);
101107

102-
MiniBlockCompressed {
108+
Ok(MiniBlockCompressed {
103109
chunks,
104110
data: vec![data_buffer],
105111
num_values: data.num_values,
106-
}
112+
})
107113
}
108114
}
109115

@@ -177,7 +183,7 @@ impl ValueEncoder {
177183
data: FixedWidthDataBlock,
178184
layers: Vec<MiniblockFslLayer>,
179185
num_rows: u64,
180-
) -> (MiniBlockCompressed, CompressiveEncoding) {
186+
) -> Result<(MiniBlockCompressed, CompressiveEncoding)> {
181187
// Count size to calculate rows per chunk
182188
let mut ceil_bytes_validity = 0;
183189
let mut cum_dim = 1;
@@ -198,7 +204,7 @@ impl ValueEncoder {
198204
};
199205
let est_bytes_per_word = (ceil_bytes_validity * vals_per_word) + cum_bytes_per_word;
200206
let (log_rows_per_chunk, rows_per_chunk) =
201-
Self::find_log_vals_per_chunk(est_bytes_per_word, vals_per_word);
207+
Self::find_log_vals_per_chunk(est_bytes_per_word, vals_per_word)?;
202208

203209
let num_chunks = num_rows.div_ceil(rows_per_chunk) as usize;
204210

@@ -258,17 +264,17 @@ impl ValueEncoder {
258264
.chain(std::iter::once(data.data))
259265
.collect::<Vec<_>>();
260266

261-
(
267+
Ok((
262268
MiniBlockCompressed {
263269
chunks,
264270
data: buffers,
265271
num_values: num_rows,
266272
},
267273
encoding,
268-
)
274+
))
269275
}
270276

271-
fn miniblock_fsl(data: DataBlock) -> (MiniBlockCompressed, CompressiveEncoding) {
277+
fn miniblock_fsl(data: DataBlock) -> Result<(MiniBlockCompressed, CompressiveEncoding)> {
272278
let num_rows = data.num_values();
273279
let fsl = data.as_fixed_size_list().unwrap();
274280
let mut layers = Vec::new();
@@ -469,9 +475,9 @@ impl MiniBlockCompressor for ValueEncoder {
469475
match chunk {
470476
DataBlock::FixedWidth(fixed_width) => {
471477
let encoding = ProtobufUtils21::flat(fixed_width.bits_per_value, None);
472-
Ok((Self::chunk_data(fixed_width), encoding))
478+
Ok((Self::chunk_data(fixed_width)?, encoding))
473479
}
474-
DataBlock::FixedSizeList(_) => Ok(Self::miniblock_fsl(chunk)),
480+
DataBlock::FixedSizeList(_) => Self::miniblock_fsl(chunk),
475481
_ => Err(Error::invalid_input_source(
476482
format!(
477483
"Cannot compress a data block of type {} with ValueEncoder",
@@ -989,6 +995,59 @@ mod tests {
989995
assert_eq!(decompressed.as_ref(), &sample_list);
990996
}
991997

998+
fn wide_fixed_size_binary() -> ArrayRef {
999+
let wide_value = vec![0xABu8; 5000];
1000+
Arc::new(
1001+
arrow_array::FixedSizeBinaryArray::try_from_sparse_iter_with_size(
1002+
std::iter::repeat_n(Some(wide_value.as_slice()), 4),
1003+
5000,
1004+
)
1005+
.unwrap(),
1006+
)
1007+
}
1008+
1009+
fn wide_fixed_size_list_bool() -> ArrayRef {
1010+
// A wide FSL<Boolean> is sub-byte, so it chunks eight values per word and the
1011+
// smallest unit is 16 values rather than 2.
1012+
let dimension = 4095;
1013+
let values = arrow_array::BooleanArray::from(vec![false; dimension * 2]);
1014+
let field = Arc::new(Field::new("item", DataType::Boolean, true));
1015+
Arc::new(FixedSizeListArray::new(
1016+
field,
1017+
dimension as i32,
1018+
Arc::new(values),
1019+
None,
1020+
))
1021+
}
1022+
1023+
#[rstest::rstest]
1024+
#[case::fixed_size_binary(wide_fixed_size_binary(), 2)]
1025+
#[case::fixed_size_list_bool(wide_fixed_size_list_bool(), 16)]
1026+
fn test_wide_value_miniblock_returns_error(
1027+
#[case] array: ArrayRef,
1028+
#[case] expected_min_values: u64,
1029+
) {
1030+
let starting_data = DataBlock::from_array(array);
1031+
1032+
let encoder = ValueEncoder::default();
1033+
let result = MiniBlockCompressor::compress(&encoder, starting_data);
1034+
1035+
let err = result.expect_err("wide values should not be encodable as miniblock");
1036+
assert!(
1037+
matches!(err, lance_core::Error::InvalidInput { .. }),
1038+
"expected InvalidInput, got {err:?}"
1039+
);
1040+
let msg = err.to_string();
1041+
assert!(
1042+
msg.contains("too wide for miniblock encoding"),
1043+
"unexpected error message: {msg}"
1044+
);
1045+
assert!(
1046+
msg.contains(&format!("{expected_min_values} values require")),
1047+
"unexpected error message: {msg}"
1048+
);
1049+
}
1050+
9921051
#[test]
9931052
fn test_fsl_value_compression_per_value() {
9941053
let sample_list = create_simple_fsl();

0 commit comments

Comments
 (0)