Skip to content

Commit 1e348f4

Browse files
yanghuayanghua
committed
address review comments
1 parent 067240c commit 1e348f4

File tree

6 files changed

+33
-32
lines changed

6 files changed

+33
-32
lines changed

‎rust/lance-core/src/utils/mask.rs‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -245,9 +245,9 @@ impl RowIdMask {
245245
/// list contains "full fragment" blocks but that would require some
246246
/// extra logic.
247247
pub fn iter_ids(&self) -> Option<Box<dyn Iterator<Item = RowAddress> + '_>> {
248-
if let Some(mut allow_iter) = self.allow_list.as_ref().and_then(|list| list.row_ids()) {
248+
if let Some(mut allow_iter) = self.allow_list.as_ref().and_then(|list| list.row_addrs()) {
249249
if let Some(block_list) = &self.block_list {
250-
if let Some(block_iter) = block_list.row_ids() {
250+
if let Some(block_iter) = block_list.row_addrs() {
251251
let mut block_iter = block_iter.peekable();
252252
Some(Box::new(iter::from_fn(move || {
253253
for allow_id in allow_iter.by_ref() {
@@ -360,8 +360,9 @@ impl std::ops::BitOr for RowIdMask {
360360
}
361361
}
362362

363-
/// A collection of row addresses and row ids(for stable row id mode,
364-
/// and would split it into another structure).
363+
/// A collection of row addresses.
364+
///
365+
/// Note: For stable row id mode, this may be split into a separate structure in the future.
365366
///
366367
/// These row ids may either be stable-style (where they can be an incrementing
367368
/// u64 sequence) or address style, where they are a fragment id and a row offset.
@@ -441,11 +442,11 @@ impl RowAddrTreeMap {
441442
.try_fold(0_u64, |acc, next| next.map(|next| next + acc))
442443
}
443444

444-
/// An iterator of row ids
445+
/// An iterator of row addrs
445446
///
446447
/// If there are any "full fragment" items then this can't be calculated and None
447448
/// is returned
448-
pub fn row_ids(&self) -> Option<impl Iterator<Item = RowAddress> + '_> {
449+
pub fn row_addrs(&self) -> Option<impl Iterator<Item = RowAddress> + '_> {
449450
let inner_iters = self
450451
.inner
451452
.iter()

‎rust/lance-index/src/frag_reuse.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ impl FragReuseIndex {
246246
}
247247

248248
pub fn remap_row_addrs_tree_map(&self, row_addrs: &RowAddrTreeMap) -> RowAddrTreeMap {
249-
RowAddrTreeMap::from_iter(row_addrs.row_ids().unwrap().filter_map(|addr| {
249+
RowAddrTreeMap::from_iter(row_addrs.row_addrs().unwrap().filter_map(|addr| {
250250
let addr_as_u64 = u64::from(addr);
251251
self.remap_row_id(addr_as_u64)
252252
}))

‎rust/lance-index/src/scalar/bitmap.rs‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,7 @@ impl ScalarIndex for BitmapIndex {
509509
for key in self.index_map.keys() {
510510
let bitmap = self.load_bitmap(key, None).await?;
511511
let remapped_bitmap =
512-
RowAddrTreeMap::from_iter(bitmap.row_ids().unwrap().filter_map(|addr| {
512+
RowAddrTreeMap::from_iter(bitmap.row_addrs().unwrap().filter_map(|addr| {
513513
let addr_as_u64 = u64::from(addr);
514514
mapping
515515
.get(&addr_as_u64)
@@ -521,7 +521,7 @@ impl ScalarIndex for BitmapIndex {
521521

522522
if !self.null_map.is_empty() {
523523
let remapped_null =
524-
RowAddrTreeMap::from_iter(self.null_map.row_ids().unwrap().filter_map(|addr| {
524+
RowAddrTreeMap::from_iter(self.null_map.row_addrs().unwrap().filter_map(|addr| {
525525
let addr_as_u64 = u64::from(addr);
526526
mapping
527527
.get(&addr_as_u64)
@@ -835,7 +835,7 @@ pub mod tests {
835835
// Verify results
836836
let expected_red_rows = vec![0u64, 3, 6, 10, 11];
837837
if let SearchResult::Exact(row_ids) = result {
838-
let mut actual: Vec<u64> = row_ids.row_ids().unwrap().map(|id| id.into()).collect();
838+
let mut actual: Vec<u64> = row_ids.row_addrs().unwrap().map(|id| id.into()).collect();
839839
actual.sort();
840840
assert_eq!(actual, expected_red_rows);
841841
} else {
@@ -845,7 +845,7 @@ pub mod tests {
845845
// Test 2: Search for "red" again - should hit cache
846846
let result = index.search(&query, &NoOpMetricsCollector).await.unwrap();
847847
if let SearchResult::Exact(row_ids) = result {
848-
let mut actual: Vec<u64> = row_ids.row_ids().unwrap().map(|id| id.into()).collect();
848+
let mut actual: Vec<u64> = row_ids.row_addrs().unwrap().map(|id| id.into()).collect();
849849
actual.sort();
850850
assert_eq!(actual, expected_red_rows);
851851
}
@@ -859,7 +859,7 @@ pub mod tests {
859859

860860
let expected_range_rows = vec![1u64, 2, 5, 7, 8, 12, 13];
861861
if let SearchResult::Exact(row_ids) = result {
862-
let mut actual: Vec<u64> = row_ids.row_ids().unwrap().map(|id| id.into()).collect();
862+
let mut actual: Vec<u64> = row_ids.row_addrs().unwrap().map(|id| id.into()).collect();
863863
actual.sort();
864864
assert_eq!(actual, expected_range_rows);
865865
}
@@ -873,7 +873,7 @@ pub mod tests {
873873

874874
let expected_in_rows = vec![0u64, 3, 4, 6, 9, 10, 11, 14];
875875
if let SearchResult::Exact(row_ids) = result {
876-
let mut actual: Vec<u64> = row_ids.row_ids().unwrap().map(|id| id.into()).collect();
876+
let mut actual: Vec<u64> = row_ids.row_addrs().unwrap().map(|id| id.into()).collect();
877877
actual.sort();
878878
assert_eq!(actual, expected_in_rows);
879879
}
@@ -972,11 +972,11 @@ pub mod tests {
972972
.unwrap_or_else(|_| panic!("Key {} should exist", key_val));
973973

974974
// Convert RowAddrTreeMap to a vector for easier assertion
975-
let row_ids: Vec<u64> = bitmap.row_ids().unwrap().map(u64::from).collect();
975+
let row_addrs: Vec<u64> = bitmap.row_addrs().unwrap().map(u64::from).collect();
976976

977977
// Verify length
978978
assert_eq!(
979-
row_ids.len(),
979+
row_addrs.len(),
980980
per_bitmap_size as usize,
981981
"Bitmap for key {} has wrong size",
982982
key_val
@@ -985,7 +985,7 @@ pub mod tests {
985985
// Verify first few and last few elements
986986
for i in 0..5.min(per_bitmap_size) {
987987
assert!(
988-
row_ids.contains(&i),
988+
row_addrs.contains(&i),
989989
"Bitmap for key {} should contain row_id {}",
990990
key_val,
991991
i
@@ -994,7 +994,7 @@ pub mod tests {
994994

995995
for i in (per_bitmap_size - 5)..per_bitmap_size {
996996
assert!(
997-
row_ids.contains(&i),
997+
row_addrs.contains(&i),
998998
"Bitmap for key {} should contain row_id {}",
999999
key_val,
10001000
i
@@ -1004,15 +1004,15 @@ pub mod tests {
10041004
// Verify exact range
10051005
let expected_range: Vec<u64> = (0..per_bitmap_size).collect();
10061006
assert_eq!(
1007-
row_ids, expected_range,
1007+
row_addrs, expected_range,
10081008
"Bitmap for key {} doesn't contain expected values",
10091009
key_val
10101010
);
10111011

10121012
tracing::info!(
10131013
"✓ Verified bitmap for key {}: {} rows as expected",
10141014
key_val,
1015-
row_ids.len()
1015+
row_addrs.len()
10161016
);
10171017
}
10181018

@@ -1102,7 +1102,7 @@ pub mod tests {
11021102
.get_with_key::<BitmapKey>(&cache_key_red)
11031103
.await
11041104
.unwrap();
1105-
let red_rows: Vec<u64> = cached_red.row_ids().unwrap().map(u64::from).collect();
1105+
let red_rows: Vec<u64> = cached_red.row_addrs().unwrap().map(u64::from).collect();
11061106
assert_eq!(red_rows, vec![0, 3, 6, 10, 11]);
11071107

11081108
// Call prewarm again - should be idempotent
@@ -1113,7 +1113,7 @@ pub mod tests {
11131113
.get_with_key::<BitmapKey>(&cache_key_red)
11141114
.await
11151115
.unwrap();
1116-
let red_rows_2: Vec<u64> = cached_red_2.row_ids().unwrap().map(u64::from).collect();
1116+
let red_rows_2: Vec<u64> = cached_red_2.row_addrs().unwrap().map(u64::from).collect();
11171117
assert_eq!(red_rows_2, vec![0, 3, 6, 10, 11]);
11181118
}
11191119

@@ -1228,7 +1228,7 @@ pub mod tests {
12281228
];
12291229
let actual_null_addrs: Vec<u64> = reloaded_idx
12301230
.null_map
1231-
.row_ids()
1231+
.row_addrs()
12321232
.unwrap()
12331233
.map(u64::from)
12341234
.collect();
@@ -1244,7 +1244,7 @@ pub mod tests {
12441244
.await
12451245
.unwrap();
12461246
if let crate::scalar::SearchResult::Exact(row_ids) = result {
1247-
let mut actual: Vec<u64> = row_ids.row_ids().unwrap().map(u64::from).collect();
1247+
let mut actual: Vec<u64> = row_ids.row_addrs().unwrap().map(u64::from).collect();
12481248
actual.sort();
12491249
let expected: Vec<u64> = vec![
12501250
RowAddress::new_from_parts(3, 2).into(),
@@ -1260,7 +1260,7 @@ pub mod tests {
12601260
.await
12611261
.unwrap();
12621262
if let crate::scalar::SearchResult::Exact(row_ids) = result {
1263-
let mut actual: Vec<u64> = row_ids.row_ids().unwrap().map(u64::from).collect();
1263+
let mut actual: Vec<u64> = row_ids.row_addrs().unwrap().map(u64::from).collect();
12641264
actual.sort();
12651265
let expected: Vec<u64> = vec![
12661266
RowAddress::new_from_parts(3, 4).into(),
@@ -1276,7 +1276,7 @@ pub mod tests {
12761276
.await
12771277
.unwrap();
12781278
if let crate::scalar::SearchResult::Exact(row_ids) = result {
1279-
let mut actual: Vec<u64> = row_ids.row_ids().unwrap().map(u64::from).collect();
1279+
let mut actual: Vec<u64> = row_ids.row_addrs().unwrap().map(u64::from).collect();
12801280
actual.sort();
12811281
assert_eq!(
12821282
actual, expected_null_addrs,

‎rust/lance-index/src/scalar/lance_format.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1444,8 +1444,8 @@ pub mod tests {
14441444
assert!(result.is_exact());
14451445
let row_addrs = result.row_addrs();
14461446

1447-
let row_ids_set = row_addrs
1448-
.row_ids()
1447+
let row_addrs_set = row_addrs
1448+
.row_addrs()
14491449
.unwrap()
14501450
.map(u64::from)
14511451
.collect::<std::collections::HashSet<_>>();
@@ -1459,7 +1459,7 @@ pub mod tests {
14591459
let list = list.unwrap();
14601460
let row_id = row_id.unwrap();
14611461
let vals = list.as_primitive::<UInt8Type>().values();
1462-
if row_ids_set.contains(&row_id) {
1462+
if row_addrs_set.contains(&row_id) {
14631463
assert!(match_fn(vals));
14641464
} else {
14651465
assert!(no_match_fn(vals));

‎rust/lance/src/io/commit/conflict_resolver.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1406,7 +1406,7 @@ impl<'a> TransactionRebase<'a> {
14061406
let conflicting_rows = existing_deletions.clone() & affected_rows.clone();
14071407
if conflicting_rows.len().map(|v| v > 0).unwrap_or(true) {
14081408
let sample_addressed = conflicting_rows
1409-
.row_ids()
1409+
.row_addrs()
14101410
.unwrap()
14111411
.take(5)
14121412
.collect::<Vec<_>>();

‎rust/lance/src/io/exec/scalar_index.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ impl MapIndexExec {
326326

327327
let allow_list =
328328
allow_list
329-
.row_ids()
329+
.row_addrs()
330330
.ok_or(datafusion::error::DataFusionError::External(
331331
"IndexedLookupExec: row addresses didn't have an iterable allow list"
332332
.into(),
@@ -611,7 +611,7 @@ async fn row_ids_for_mask(
611611
(Some(mut allow_list), None) => {
612612
retain_fragments(&mut allow_list, fragments, dataset).await?;
613613

614-
if let Some(allow_list_iter) = allow_list.row_ids() {
614+
if let Some(allow_list_iter) = allow_list.row_addrs() {
615615
Ok(allow_list_iter.map(u64::from).collect::<Vec<_>>())
616616
} else {
617617
// We shouldn't hit this branch if the row ids are stable.
@@ -649,7 +649,7 @@ async fn row_ids_for_mask(
649649
// We need to filter out irrelevant fragments as well.
650650
retain_fragments(&mut allow_list, fragments, dataset).await?;
651651

652-
if let Some(allow_list_iter) = allow_list.row_ids() {
652+
if let Some(allow_list_iter) = allow_list.row_addrs() {
653653
Ok(allow_list_iter
654654
.filter_map(|addr| {
655655
let row_id = u64::from(addr);

0 commit comments

Comments
 (0)