-
Notifications
You must be signed in to change notification settings - Fork 46
feat(index): support distributed LABEL_LIST index #5262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
05a91e2
bf8fd63
73b089e
f39a394
0f39468
09eb31a
62799fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -583,7 +583,7 @@ def test_build_distributed_nested_scalar_indexes(self, temp_dir): | |
| indices = {idx.name: idx for idx in updated_dataset.describe_indices()} | ||
| assert indices["nested_text_idx"].field_names == ["meta.text"] | ||
| assert indices["literal_dot_text_idx"].field_names == ["meta.`a.b`"] | ||
| assert indices["hyphen_user_id_idx"].field_names == ["`meta-data`.`user-id`"] | ||
| assert indices["hyphen_user_id_idx"].field_names == ["meta-data.user-id"] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Must we remove the ` character?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, for this assertion. This change comes from upstream: lance-format/lance#7503. |
||
|
|
||
| nested_results = updated_dataset.scanner( | ||
| full_text_query="nestedthree", | ||
|
|
@@ -1395,6 +1395,29 @@ class TestDistributedScalarSegmentIndexes: | |
| ["value = 4", "value IN (1, 6, 11)"], | ||
| id="bloomfilter", | ||
| ), | ||
| pytest.param( | ||
| "LABEL_LIST", | ||
| "labels", | ||
| [ | ||
| ["distributed", "shared"], | ||
| ["other", None], | ||
| None, | ||
| [], | ||
| ["distributed"], | ||
| ["shared", "other"], | ||
| [None], | ||
| ["other"], | ||
| ["distributed", "shared"], | ||
| ["other", None], | ||
| None, | ||
| [], | ||
| ], | ||
| pa.large_list(pa.string()), | ||
| "labels_idx", | ||
| "LabelList", | ||
| ["array_has_any(labels, ['distributed'])"], | ||
| id="label-list", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_filter_index_matches_baseline( | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly added LABEL_LIST type validation is only executed when index_type is a string. If the caller passes in an IndexConfig object in accordance with the public contract, this early validation will be skipped. As a result, invalid column types cannot be detected in advance on the Driver side, and failures may be deferred until the Ray worker execution phase. right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that’s right. The existing driver-side checks for the other scalar indexes also only run when
index_typeis a string;IndexConfigskips them today. This PR follows the existing pattern and keeps the change focused on addingLABEL_LIST. I’m considering aligning validation for string and IndexConfig inputs in lance-format/lance-ray#5250, rather than mixing that broader change into this PR.