feat(python): expose LABEL_LIST segment index build - #7884
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLABEL_LIST scalar indexes now support both list and large-list Arrow columns, distributed segment construction, query parsing, UUID safeguards, and dataset-level indexed execution tests. ChangesLABEL_LIST indexing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LanceDataset
participant create_index_uncommitted
participant merge_existing_index_segments
participant commit_existing_index_segments
LanceDataset->>create_index_uncommitted: create per-fragment LABEL_LIST segments
create_index_uncommitted->>merge_existing_index_segments: provide index segments
merge_existing_index_segments->>commit_existing_index_segments: commit merged segments
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
A-python
105a368 to
0537d73
Compare
0537d73 to
3307d27
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
python/python/tests/test_scalar_index.py-4642-4706 (1)
4642-4706: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover
array_has_allwith multiple labels.This only exercises the new large-list path through
array_has_any; the distinctHasAllLabelspath remains untested. Parameterize the predicate and add multi-label rows plus anarray_has_all(labels, [...])case that distinguishes all-label matching.As per coding guidelines, “Every bugfix and feature must have corresponding tests,” and Python tests differing only by inputs should use
@pytest.mark.parametrize.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/python/tests/test_scalar_index.py` around lines 4642 - 4706, Extend test_label_list_segment_index to parameterize the filter predicate and expected matching data, adding multi-label rows and an array_has_all(labels, [...]) case that distinguishes all-label matching from array_has_any. Keep coverage for both list and large_list label types, and use pytest.mark.parametrize for the input-only variations while preserving the indexed versus non-indexed result and ScalarIndexQuery assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/python/lance/dataset.py`:
- Around line 3237-3242: Remove the Python-side list/large-list type check and
its TypeError from the LABEL_LIST index handling path in the relevant dataset
method. Keep only argument normalization and routing there, allowing the Rust
implementation to validate column eligibility and produce the descriptive
rejection error.
---
Other comments:
In `@python/python/tests/test_scalar_index.py`:
- Around line 4642-4706: Extend test_label_list_segment_index to parameterize
the filter predicate and expected matching data, adding multi-label rows and an
array_has_all(labels, [...]) case that distinguishes all-label matching from
array_has_any. Keep coverage for both list and large_list label types, and use
pytest.mark.parametrize for the input-only variations while preserving the
indexed versus non-indexed result and ScalarIndexQuery assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 0a0efff7-fde4-455b-ad19-95e4d4bc12c6
📒 Files selected for processing (3)
python/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/lance-index/src/scalar/expression.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
3307d27 to
26e0a6c
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
rust/lance/src/dataset/tests/dataset_index.rs-2911-2927 (1)
2911-2927: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrefix the boolean with
is_.Rename
large_listtois_large_list.As per coding guidelines, “Prefix boolean variables with
is_orhas_instead of ambiguouswith_or bare adjectives.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/tests/dataset_index.rs` around lines 2911 - 2927, Rename the boolean parameter in test_label_list_index_types from large_list to is_large_list and update its conditional usage accordingly.Source: Coding guidelines
python/python/lance/dataset.py-3309-3309 (1)
3309-3309: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the LABEL_LIST distributed-build exception.
This change makes
create_scalar_index(..., index_type="LABEL_LIST", fragment_ids=...)raise and requirecreate_index_uncommitted, while Line 3425 still saysfragment_idsreturns an uncommitted segment. Update that public docstring to direct LABEL_LIST callers tocreate_index_uncommitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/python/lance/dataset.py` at line 3309, Update the public docstring for create_scalar_index to document that index_type="LABEL_LIST" does not support fragment_ids in distributed builds and raises instead; direct callers needing this path to use create_index_uncommitted, correcting the conflicting statement near the fragment_ids documentation.python/python/lance/dataset.py-3240-3242 (1)
3240-3242: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the rejected column type in the error.
The new validation reports the column name but not
field_type, making invalid schemas harder to diagnose.Proposed fix
raise TypeError( - f"LABEL_LIST index column {column} must be a list or large list" + f"LABEL_LIST index column {column} must be a list or large list, " + f"got field_type={field_type}" )As per coding guidelines, include full error context, including variable names, values, sizes, and types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/python/lance/dataset.py` around lines 3240 - 3242, Update the TypeError raised by the LABEL_LIST index-column validation to include the rejected field_type value and type, while retaining the column name and existing list/large-list requirement. Modify the validation error in the surrounding schema-checking logic without changing validation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@python/python/lance/dataset.py`:
- Line 3309: Update the public docstring for create_scalar_index to document
that index_type="LABEL_LIST" does not support fragment_ids in distributed builds
and raises instead; direct callers needing this path to use
create_index_uncommitted, correcting the conflicting statement near the
fragment_ids documentation.
- Around line 3240-3242: Update the TypeError raised by the LABEL_LIST
index-column validation to include the rejected field_type value and type, while
retaining the column name and existing list/large-list requirement. Modify the
validation error in the surrounding schema-checking logic without changing
validation behavior.
In `@rust/lance/src/dataset/tests/dataset_index.rs`:
- Around line 2911-2927: Rename the boolean parameter in
test_label_list_index_types from large_list to is_large_list and update its
conditional usage accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: a6f814bb-9bd4-4262-abb0-ab4adb78f690
📒 Files selected for processing (4)
python/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/lance-index/src/scalar/expression.rsrust/lance/src/dataset/tests/dataset_index.rs
26e0a6c to
b1a13ce
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
rust/lance-index/src/scalar/expression.rs-1676-1682 (1)
1676-1682: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid
.expect()for this invariant; usedebug_assert!+ graceful fallback instead.
rebuild_and_tree(leaves)can only returnNonewhenleavesis empty, which cannot happen here sincecollect_and_leavesis only invoked on anAndnode and always pushes at least one leaf. That said, panicking on a violated invariant in production code contradicts the repo guideline to not silently guard impossible conditions with a panic-on-failure call.🛡️ Proposed fix
- Self::rebuild_and_tree(leaves).expect("AND tree optimization should keep at least one leaf") + match Self::rebuild_and_tree(leaves) { + Some(tree) => tree, + None => { + debug_assert!(false, "AND tree optimization should keep at least one leaf"); + self_before_flattening + } + }Note: since
selfis consumed bycollect_and_leaves, the fallback branch would need to retain a clone of the original expression (or simply return a degenerate leaf) rather than referencingselfafter the move.As per coding guidelines, "Do not silently guard against impossible conditions; use
debug_assert!, return an explicit error, or remove the check."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/expression.rs` around lines 1676 - 1682, The optimize_and_tree method should not use expect on rebuild_and_tree. Add a debug_assert! that the optimized leaves are non-empty, then handle a None result gracefully by returning a valid fallback expression, such as a retained clone of the original expression or a degenerate leaf; preserve the optimized tree path when rebuilding succeeds.Source: Coding guidelines
python/python/lance/dataset.py-400-406 (1)
400-406: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd coverage for materialized merge insert source dispatch.
python/python/lance/dataset.pyroutes_is_materialized(data_obj)in bothexecute()andexecute_uncommitted()to the batch-backed APIs at lines 403-406 and 430-433, but the Python tests do not exerciseexecute_batches/execute_uncommitted_batchesfor merge insert. Add a test for a materialized source such aspa.Tableordictwith conflict retries to catch regressions in retry/spill behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/python/lance/dataset.py` around lines 400 - 406, Add Python test coverage for MergeInsertBuilder dispatch when data_obj is materialized, such as a pa.Table or dict. Exercise both execute() and execute_uncommitted() with conflict retries, asserting they use execute_batches() and execute_uncommitted_batches() respectively and preserve retry behavior without spilling.
🧹 Nitpick comments (1)
rust/lance/src/dataset/tests/dataset_index.rs (1)
2907-2995: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd multi-fragment and NULL-label coverage to this LabelList index test.
test_label_list_index_typeswrites all 4 rows in a single write (one fragment) and every row has a non-null label list. The repo's index-test guideline specifically calls for multi-fragment scenarios and NULL edge cases; without a NULLlabelsentry or a multi-fragment dataset, this test won't catch a bug in LabelList's handling of null lists or of merging label data across fragments forLargeList(a newly-supported type here).🧪 Suggested additions
- let label_values = vec![ - Some(vec![Some(1), Some(2)]), - Some(vec![Some(2)]), - Some(vec![Some(1)]), - Some(vec![Some(3)]), - ]; + let label_values = vec![ + Some(vec![Some(1), Some(2)]), + Some(vec![Some(2)]), + Some(vec![Some(1)]), + Some(vec![Some(3)]), + None, // NULL label list row + ];and write with
max_rows_per_fileset to force multiple fragments, similar to the Pythontest_label_list_segment_indextest in this same PR.As per coding guidelines, "Include multi-fragment dataset scenarios, NULL edge cases in index tests, vector-index recall assertions of at least 0.5."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/tests/dataset_index.rs` around lines 2907 - 2995, Extend test_label_list_index_types to include at least one NULL labels value and write the batch with Dataset::write options, including max_rows_per_file, that force multiple fragments. Keep both List and LargeList cases, and retain the pre-index versus post-index result and plan assertions so LabelList filtering is validated across fragments and null lists.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@python/python/lance/dataset.py`:
- Around line 400-406: Add Python test coverage for MergeInsertBuilder dispatch
when data_obj is materialized, such as a pa.Table or dict. Exercise both
execute() and execute_uncommitted() with conflict retries, asserting they use
execute_batches() and execute_uncommitted_batches() respectively and preserve
retry behavior without spilling.
In `@rust/lance-index/src/scalar/expression.rs`:
- Around line 1676-1682: The optimize_and_tree method should not use expect on
rebuild_and_tree. Add a debug_assert! that the optimized leaves are non-empty,
then handle a None result gracefully by returning a valid fallback expression,
such as a retained clone of the original expression or a degenerate leaf;
preserve the optimized tree path when rebuilding succeeds.
---
Nitpick comments:
In `@rust/lance/src/dataset/tests/dataset_index.rs`:
- Around line 2907-2995: Extend test_label_list_index_types to include at least
one NULL labels value and write the batch with Dataset::write options, including
max_rows_per_file, that force multiple fragments. Keep both List and LargeList
cases, and retain the pre-index versus post-index result and plan assertions so
LabelList filtering is validated across fragments and null lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: ae7f1de3-722e-426b-bb27-3d2dc59bcebf
📒 Files selected for processing (4)
python/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/lance-index/src/scalar/expression.rsrust/lance/src/dataset/tests/dataset_index.rs
|
Could @jackye1995 @Xuanwo help review this? thanks |
b1a13ce to
e743181
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
rust/lance/src/dataset/tests/dataset_index.rs-2913-2918 (1)
2913-2918: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd null-list and null-element cases.
The schema is nullable, but every test value is non-null. Include
Noneand a list containingNone, then retain the baseline-versus-index result comparison.As per coding guidelines, “Include multi-fragment dataset scenarios, NULL edge cases in index tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/tests/dataset_index.rs` around lines 2913 - 2918, Update the label_values test fixture to include both a null list (None) and a non-null list containing a null element, while retaining the existing non-null cases. Preserve the baseline-versus-index result comparison so these nullable inputs are validated through both query paths.Source: Coding guidelines
rust/lance-index/src/scalar/expression.rs-2770-2800 (1)
2770-2800: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover
array_has_allfor both list widths.This only verifies the
HasAnyLabelbranch. Add anarray_has_all(labels, [...])case with multiple labels and assertHasAllLabels.As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/expression.rs` around lines 2770 - 2800, Extend test_label_list_query_parser for both List and LargeList cases by adding an array_has_all(labels, [...]) query containing multiple labels. Assert it produces IndexedExpression::index_query with LabelListQuery::HasAllLabels containing the expected label values, preserving the existing HasAnyLabel coverage.Source: Coding guidelines
python/python/lance/dataset.py-3259-3261 (1)
3259-3261: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the rejected Arrow type in the error.
The validation error identifies
columnbut omits the receivedfield_type, making unsupported schemas harder to diagnose.Proposed fix
- f"LABEL_LIST index column {column} must be a list or large list" + f"LABEL_LIST index column {column} must be a list or large list; " + f"got {field_type}"As per coding guidelines, “Include full error context, including variable names, values, sizes, and types.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/python/lance/dataset.py` around lines 3259 - 3261, Update the LABEL_LIST index-column validation error in the surrounding schema validation logic to include the rejected field_type value and type context alongside column, while preserving the existing TypeError behavior and message intent.Source: Coding guidelines
🧹 Nitpick comments (2)
rust/lance/src/dataset/tests/dataset_index.rs (2)
2907-2911: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the boolean to
is_large_list.
large_listis a boolean parameter and should use anis_/has_prefix.As per coding guidelines, “Prefix boolean variables with
is_orhas_instead of ambiguouswith_or bare adjectives.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/tests/dataset_index.rs` around lines 2907 - 2911, Rename the boolean parameter in test_label_list_index_types from large_list to is_large_list, and update all references within the test and its cases or setup logic to use the new name consistently.Source: Coding guidelines
2932-2937: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
record_batch!for test setup.Avoid manual schema and
RecordBatch::try_newboilerplate here.As per coding guidelines, “Use
record_batch!()fromarrow_arrayto constructRecordBatchin tests instead of manualSchema/Arc/try_newboilerplate.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/tests/dataset_index.rs` around lines 2932 - 2937, Update the test setup around RecordBatchIterator::new to construct the batch with arrow_array’s record_batch! macro instead of manually passing schema.clone(), Arc arrays, and RecordBatch::try_new. Preserve the existing column values and labels, and continue supplying the resulting batch and schema to the iterator as required.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@python/python/lance/dataset.py`:
- Around line 3259-3261: Update the LABEL_LIST index-column validation error in
the surrounding schema validation logic to include the rejected field_type value
and type context alongside column, while preserving the existing TypeError
behavior and message intent.
In `@rust/lance-index/src/scalar/expression.rs`:
- Around line 2770-2800: Extend test_label_list_query_parser for both List and
LargeList cases by adding an array_has_all(labels, [...]) query containing
multiple labels. Assert it produces IndexedExpression::index_query with
LabelListQuery::HasAllLabels containing the expected label values, preserving
the existing HasAnyLabel coverage.
In `@rust/lance/src/dataset/tests/dataset_index.rs`:
- Around line 2913-2918: Update the label_values test fixture to include both a
null list (None) and a non-null list containing a null element, while retaining
the existing non-null cases. Preserve the baseline-versus-index result
comparison so these nullable inputs are validated through both query paths.
---
Nitpick comments:
In `@rust/lance/src/dataset/tests/dataset_index.rs`:
- Around line 2907-2911: Rename the boolean parameter in
test_label_list_index_types from large_list to is_large_list, and update all
references within the test and its cases or setup logic to use the new name
consistently.
- Around line 2932-2937: Update the test setup around RecordBatchIterator::new
to construct the batch with arrow_array’s record_batch! macro instead of
manually passing schema.clone(), Arc arrays, and RecordBatch::try_new. Preserve
the existing column values and labels, and continue supplying the resulting
batch and schema to the iterator as required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 04d01c95-69c2-4bc4-982d-e8c73790fdca
📒 Files selected for processing (4)
python/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/lance-index/src/scalar/expression.rsrust/lance/src/dataset/tests/dataset_index.rs
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
I found four correctness issues that make this unsafe to merge. The LargeList compatibility/type issues and the live-UUID overwrite are attached inline. One cross-file transaction issue remains:
A staged LABEL_LIST segment can be committed from a stale dataset handle after a concurrent RewriteColumns changes the indexed field. Source-snapshot pruning runs against the stale handle before the transaction, check_create_index_txn accepts the concurrent Update, and finish_create_index does not revalidate coverage against the latest manifest.
On this head, I staged and merged two fragments at v1, changed row 0 from old to new at v2, then committed from the stale v1 handle. The commit succeeded with coverage [[0, 1]]; old was flat [2] versus indexed [0, 2], while new was flat [0] versus indexed []. Committing from the updated handle correctly pruned coverage to [[1]], and a concurrent append control remained correct via flat fallback, so the gap is specifically the overlapping rewrite window.
A transaction-level regression could require a conflict when fields_modified and fragment coverage overlap, or verify that provenance pruning is rerun against the latest manifest before publication.
| } | ||
| let list_values = match label_list { | ||
| ScalarValue::List(list_arr) => list_arr.values().clone(), | ||
| ScalarValue::LargeList(list_arr) => list_arr.values().clone(), |
There was a problem hiding this comment.
This new exact LargeList arm makes released nullable v0 LabelList indexes return wrong rows under negation. Lance v3.0.1 could write LargeList LabelList indexes with index_version = 0 and LabelListIndexDetails, but without list-level NULL metadata. The current compatibility path only warns, and read_list_nulls treats the missing metadata as an empty NULL set, so complementing NOT array_has_any/all includes NULL-list rows.
On this head, a v0 artifact over rows [[1], NULL, [2]] produced flat = [2] but indexed = [1, 2] for NOT array_has_any(labels, [1]); the plan confirmed ScalarIndexQuery@legacy_labels(LabelList).
A bounded compatibility fix could keep nullable v0 segments on scan/recheck, or reject them, before enabling exact LargeList planning. A historical-format regression covering both negated predicates would lock this down.
| if not pa.types.is_list(field_type): | ||
| raise TypeError(f"LABEL_LIST index column {column} must be a list") | ||
| if not ( | ||
| pa.types.is_list(field_type) or pa.types.is_large_list(field_type) |
There was a problem hiding this comment.
Accepting every LargeList here expands the supported schema beyond what LabelList bitmap keys can order, so index creation can succeed and a later query can panic. LargeList<List<Int64>> passes this check; the first indexed query reaches the (List, List) => todo!() comparator in btree.rs and surfaces ArrowInvalid: Task was aborted. The flat query returns the correct row.
The following ran on this head and fails at the final call:
with tempfile.TemporaryDirectory() as uri:
ds = lance.write_dataset(
pa.table({
"id": pa.array([0, 1], type=pa.int32()),
"labels": pa.array(
[[[1, 2]], [[3]]],
type=pa.large_list(pa.list_(pa.int64())),
),
}),
uri,
)
predicate = "array_has_any(labels, [[1, 2]])"
assert ds.scanner(
filter=predicate, use_scalar_index=False
).to_table()["id"].to_pylist() == [0]
ds.create_scalar_index("labels", "LABEL_LIST")
assert "ScalarIndexQuery" in ds.scanner(filter=predicate).explain_plan()
ds.scanner(filter=predicate).to_table() # Rust panicThe comparator limitation predates this PR for nested List, but this change newly accepts the equivalent LargeList schema and makes its any/all path reachable. A bounded fix could validate supported element types in Rust before writing the artifact, mirror that check in Python, and retain scan fallback for existing unsupported indexes.
There was a problem hiding this comment.
Good catch. LABEL_LIST now rejects nested item types for both List and LargeList in Rust Core. The query parser applies the same check so legacy invalid indexes fall back to a scan instead of reaching the unsupported bitmap comparison. Both paths are covered by parameterized tests.
| "RTREE", | ||
| "ZONEMAP", | ||
| "BLOOMFILTER", | ||
| "LABEL_LIST", |
There was a problem hiding this comment.
Making LABEL_LIST segment-native exposes caller-supplied index_uuid without the collision guard used by BTree/RTree. LabelList writes _indices/<uuid>/bitmap_page_lookup.lance with replace semantics, so reusing a committed UUID overwrites a live index before any manifest commit.
On this head I published an index for labels_a, then staged labels_b with the live UUID. The dataset version stayed unchanged, but the live file hash changed. After reopening:
aaaa: flat=[0, 2, 4, 6], indexed=[]
cccc: flat=[], indexed=[0, 2, 4, 6]
This creates both false negatives and false positives without calling commit_existing_index_segments. LABEL_LIST could reject caller-supplied UUIDs before any I/O, with a regression asserting that the original artifact hash and indexed results remain unchanged after a rejected collision.
There was a problem hiding this comment.
I added a Core guard to reject caller-supplied UUIDs for distributed LABEL_LIST builds before any I/O.
e743181 to
4f0345f
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-index/src/scalar/expression.rs-2791-2804 (1)
2791-2804: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover
array_has_allforLargeList.The new
ScalarValue::LargeListextraction also feedsHasAllLabels, but this test only exercisesarray_has_any. Add anarray_has_allassertion under the existingrstestcases.Proposed test extension
- schema, + schema.clone(), ); + + check_with_schema( + &index_info, + "array_has_all(labels, ['distributed', 'replicated'])", + Some(IndexedExpression::index_query( + "labels".to_string(), + "labels_idx".to_string(), + "LabelList".to_string(), + Arc::new(LabelListQuery::HasAllLabels(vec![ + ScalarValue::Utf8(Some("distributed".to_string())), + ScalarValue::Utf8(Some("replicated".to_string())), + ])), + )), + true, + schema, + );As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/expression.rs` around lines 2791 - 2804, Add an assertion in the existing rstest cases near the array_has_any check to exercise array_has_all(labels, ['distributed']) with a LargeList-backed label index and the expected HasAllLabels query. Reuse the existing check_with_schema setup and symbols, changing only the expression and expected query variant needed to cover HasAllLabels.Source: Coding guidelines
🧹 Nitpick comments (1)
rust/lance/src/dataset/tests/dataset_index.rs (1)
2911-2965: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReplace
.default()with explicit LabelList params.
ScalarIndexParams::default()resolves toBTree; non-distributed scalar builds discard the passed params’index_typestring and always initializebase_paramsfromself.index_type, so this currently works but is still inconsistent with the rest of the LabelList tests and can be misleading for future callers. UseScalarIndexParams::for_builtin(BuiltinIndexType::LabelList)here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/tests/dataset_index.rs` around lines 2911 - 2965, Update create_index in test_label_list_index_types to pass explicit LabelList parameters using ScalarIndexParams::for_builtin(BuiltinIndexType::LabelList) instead of ScalarIndexParams::default(), matching the other LabelList tests and avoiding the BTree default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@rust/lance-index/src/scalar/expression.rs`:
- Around line 2791-2804: Add an assertion in the existing rstest cases near the
array_has_any check to exercise array_has_all(labels, ['distributed']) with a
LargeList-backed label index and the expected HasAllLabels query. Reuse the
existing check_with_schema setup and symbols, changing only the expression and
expected query variant needed to cover HasAllLabels.
---
Nitpick comments:
In `@rust/lance/src/dataset/tests/dataset_index.rs`:
- Around line 2911-2965: Update create_index in test_label_list_index_types to
pass explicit LabelList parameters using
ScalarIndexParams::for_builtin(BuiltinIndexType::LabelList) instead of
ScalarIndexParams::default(), matching the other LabelList tests and avoiding
the BTree default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 1a52359f-73c8-4685-b9bd-b9201e9df5f9
📒 Files selected for processing (6)
python/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/label_list.rsrust/lance/src/dataset/tests/dataset_index.rsrust/lance/src/index/create.rs
2 participants
Summary
Expose existing
LABEL_LISTsegment builds in Pylance.Core already supports building a
LABEL_LISTindex onLargeList, but its query parser only acceptedListliterals. ALargeListfilter therefore fell back to a regular scan instead of producingScalarIndexQuery. This change makes both list types use the same parser path and adds parser and dataset-plan coverage.Testing
cargo test -p lance test_label_list_index_typescargo test -p lance-index test_label_list_query_parseruv run --frozen pytest python/tests/test_scalar_index.py::test_label_list_segment_indexuv run --frozen pytest python/tests/test_scalar_index.py -k label_list