-
Notifications
You must be signed in to change notification settings - Fork 733
feat: clarify logical indices and physical index segments #6270
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 1 commit
1c870aa
fa8dccb
98fe15c
e166f8d
682a8e4
0962af0
8ad7263
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 |
|---|---|---|
|
|
@@ -77,6 +77,13 @@ pub trait IndexDescription: Send + Sync { | |
| /// IndexMetadata for each segment of the index. | ||
| fn metadata(&self) -> &[IndexMetadata]; | ||
|
|
||
| /// Returns the physical index segments that make up this logical index. | ||
| /// | ||
| /// This is an alias for [`Self::metadata`] with a less ambiguous name. | ||
|
Contributor
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. praise: +1 I like explicitly calling it segments. |
||
| fn segments(&self) -> &[IndexMetadata] { | ||
| self.metadata() | ||
| } | ||
|
|
||
| /// Returns the index type URL | ||
| /// | ||
| /// This is extracted from the type url of the index details | ||
|
|
@@ -210,6 +217,8 @@ pub trait DatasetIndexExt { | |
| /// | ||
| /// The indices are lazy loaded and cached in memory within the `Dataset` instance. | ||
| /// The cache is invalidated when the dataset version (Manifest) is changed. | ||
| /// | ||
| /// Each returned entry represents a physical index segment from the manifest. | ||
| async fn load_indices(&self) -> Result<Arc<Vec<IndexMetadata>>>; | ||
|
|
||
| /// Loads all the indies of a given UUID. | ||
|
|
@@ -243,6 +252,21 @@ pub trait DatasetIndexExt { | |
| }) | ||
| } | ||
|
|
||
| /// Describe physical index segments. | ||
| /// | ||
| /// When `name` is provided, only segments belonging to the named logical | ||
| /// index are returned. Otherwise, all index segments in the current dataset | ||
| /// version are returned. | ||
| async fn describe_index_segments(&self, name: Option<&str>) -> Result<Vec<IndexMetadata>> { | ||
|
Contributor
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. question(blocking): Users can already call I say this in part because I think
Collaborator
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. I think it's a good point. Let's remove it from the public API. |
||
| match name { | ||
| Some(name) => self.load_indices_by_name(name).await, | ||
| None => self | ||
| .load_indices() | ||
| .await | ||
| .map(|indices| indices.as_ref().clone()), | ||
| } | ||
| } | ||
|
|
||
| /// Loads a specific index with the given index name. | ||
| /// This function only works for indices that are unique. | ||
| /// If there are multiple indices sharing the same name, please use [`Self::load_indices_by_name`] | ||
|
|
||
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.
if we removed in Rust, should we also remove it in Python and Java? Or is there a reason to keep it in these bindings?
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.
ooops, I overlook them