Skip to content

Commit c4b4515

Browse files
wjones127claude
andcommitted
test(blob): cover mixed-nullability batches in one write
The blob encoder derives each page's definition interpretation from the validity bitmap of the batch it saw, so a single write whose batches disagree about nullability used to panic in debug builds and lose the nulls in release builds. #8070 fixed this by closing the pending descriptor page whenever the interpretation changes, but the Rust coverage stops at the encoder; the dataset write path is only covered from Python, via compaction. This adds a `Dataset::write` regression test for a nullable blob column whose batches disagree, in both orders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 389fb70 commit c4b4515

1 file changed

Lines changed: 61 additions & 0 deletions

File tree

rust/lance/src/dataset/blob.rs

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4432,6 +4432,67 @@ mod tests {
44324432
}
44334433
}
44344434

4435+
/// The blob encoder derives each page's definition interpretation from the validity
4436+
/// bitmap of the batch it saw, so a write whose batches disagree about nullability
4437+
/// used to panic in debug builds and drop the nulls in release builds
4438+
/// (https://github.com/lance-format/lance/issues/8077). Batch boundaries are not
4439+
/// under the caller's control during compaction or merge_insert, so a null must
4440+
/// survive in either order.
4441+
#[rstest]
4442+
#[tokio::test]
4443+
async fn test_write_blob_batches_with_mixed_nullability(
4444+
#[values(false, true)] nulls_first: bool,
4445+
) {
4446+
let test_dir = TempStrDir::default();
4447+
let schema = Arc::new(Schema::new(vec![
4448+
Field::new("blob", DataType::LargeBinary, true).with_metadata(HashMap::from([(
4449+
BLOB_META_KEY.to_string(),
4450+
"true".to_string(),
4451+
)])),
4452+
]));
4453+
let batch = |values: Vec<Option<&[u8]>>| {
4454+
RecordBatch::try_new(
4455+
schema.clone(),
4456+
vec![Arc::new(LargeBinaryArray::from(values))],
4457+
)
4458+
.unwrap()
4459+
};
4460+
4461+
let all_valid = batch(vec![Some(b"a".as_slice()), Some(b"".as_slice())]);
4462+
let with_null = batch(vec![Some(b"c".as_slice()), None]);
4463+
let (batches, expected) = if nulls_first {
4464+
(
4465+
vec![with_null, all_valid],
4466+
vec![Some(b"c".as_slice()), None, Some(b"a"), Some(b"")],
4467+
)
4468+
} else {
4469+
(
4470+
vec![all_valid, with_null],
4471+
vec![Some(b"a".as_slice()), Some(b""), Some(b"c"), None],
4472+
)
4473+
};
4474+
4475+
let reader = RecordBatchIterator::new(batches.into_iter().map(Ok), schema.clone());
4476+
let dataset = Arc::new(Dataset::write(reader, &test_dir, None).await.unwrap());
4477+
4478+
let blobs = dataset
4479+
.take_blobs_by_indices(&[0, 1, 2, 3], "blob")
4480+
.await
4481+
.unwrap();
4482+
assert_eq!(blobs.len(), expected.len());
4483+
for (idx, expected) in expected.into_iter().enumerate() {
4484+
match (&blobs[idx], expected) {
4485+
(Some(blob), Some(expected)) => {
4486+
assert_eq!(blob.read().await.unwrap().as_ref(), expected, "row {idx}");
4487+
}
4488+
(None, None) => {}
4489+
(actual, expected) => {
4490+
panic!("row {idx}: expected {expected:?}, got {}", actual.is_some());
4491+
}
4492+
}
4493+
}
4494+
}
4495+
44354496
#[tokio::test]
44364497
pub async fn test_take_blob_id_not_exist() {
44374498
let fixture = BlobTestFixture::new().await;

0 commit comments

Comments
 (0)