fix: preserve row address filters with stable row ids - #8145
Conversation
There was a problem hiding this comment.
Gate recommendation: request changes. The serialized row-set may remain row-ID based, but address normalization must preserve physical-address liveness: a deleted _rowaddr must not follow its stable ID to a replacement row. Use a tolerant converter that drops missing, out-of-range, and deleted physical slots before serialization, with update and stable _rowoffset regression coverage.
| } | ||
|
|
||
| async fn row_addrs_as_take_input(&self, row_addrs: Vec<u64>) -> Result<Arc<dyn ExecutionPlan>> { | ||
| let row_ids = row_addrs_to_row_ids(&self.dataset, row_addrs.into_iter().map(Some)).await?; |
There was a problem hiding this comment.
row_addrs_to_row_ids does not consult deletion vectors. If an update deletes this physical slot and writes the same stable ID into a replacement fragment, the generated ID mask matches the replacement row, so filtering on a stale _rowaddr reads the wrong logical row instead of returning no match. Convert only live physical slots (while still dropping missing and out-of-range user literals) before serialization.
Reproducer
I ran this as a test in this module on cededf0c76c03567811471fc4141b26d21374842:
#[tokio::test]
async fn old_rowaddr_does_not_follow_updated_stable_id() {
let ds = lance_datagen::gen_batch()
.col("idx", array::step::<Int32Type>())
.into_ram_dataset_with_params(
FragmentCount::from(2),
FragmentRowCount::from(3),
Some(WriteParams {
max_rows_per_file: 3,
enable_stable_row_ids: true,
..Default::default()
}),
)
.await
.unwrap();
let old_addr = u64::from(RowAddress::new_from_parts(0, 1));
let ds = crate::dataset::UpdateBuilder::new(Arc::new(ds))
.update_where("idx = 1").unwrap()
.set("idx", "101").unwrap()
.build().unwrap()
.execute().await.unwrap()
.new_dataset;
let batch = ds.scan()
.filter(&format!("{ROW_ADDR} = {old_addr}")).unwrap()
.try_into_batch().await.unwrap();
assert_eq!(batch.num_rows(), 0);
}cargo test -p lance old_rowaddr_does_not_follow_updated_stable_id -- --nocapture observed num_rows() == 1, not 0.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Gate recommendation: approve. The live-only address converter preserves the row-ID row-set contract while dropping missing, out-of-range, and deleted physical slots before serialization. The follow-up also covers update replacements and stable row offsets, so stale _rowaddr values can no longer follow stable IDs.
Summary
Root cause
The filter-to-take optimization distinguished row addresses from row IDs while parsing, but serialized both through the same row-set helper. Filtered reads interpret serialized row sets as row IDs when stable row IDs are enabled, so physical addresses were matched against the wrong domain. Address normalization must also preserve physical liveness because updates can move the same stable ID to a replacement fragment.
The separately reported count_rows pushdown error also occurs without stable row IDs and is independent of this address-domain bug.
Validation
Fixes #8126