Support Export All ID from index#1069
Merged
Merged
Conversation
LHT129
added
the
kind/feature
LHT129
requested review from
ShawnShawnYou,
jiaweizone and
wxyucs
as code owners
August 19, 2025 08:31
LHT129
added
the
version/0.17
Contributor
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
Reviewer's GuideThis PR adds comprehensive support for exporting all index IDs by introducing a new feature flag, defining the ExportIDs API across interfaces and implementations, exposing label table data, and validating the functionality with new unit tests. Entity relationship diagram for LabelTable and Dataset in ExportIDserDiagram
LABEL_TABLE {
int64_t id
LabelType label_table[]
}
DATASET {
int64_t num_elements
LabelType ids[]
int dim
bool owner
}
LABEL_TABLE ||--o| DATASET : exports IDs
Class diagram for ExportIDs API integrationclassDiagram
class Index {
+ExportIDs() : tl::expected<DatasetPtr, Error>
}
class IndexImpl {
+ExportIDs() : tl::expected<DatasetPtr, Error>
}
class InnerIndexInterface {
+ExportIDs() : DatasetPtr
}
Index <|-- IndexImpl
IndexImpl o-- InnerIndexInterface
class LabelTable {
+GetAllLabels() : const LabelType*
}
InnerIndexInterface o-- LabelTable
class Dataset {
+Make()
+NumElements()
+Ids()
+Dim()
+Owner()
}
InnerIndexInterface o-- Dataset
Class diagram for new IndexFeature SUPPORT_EXPORT_IDSclassDiagram
class IndexFeature {
<<enum>>
SUPPORT_EXPORT_IDS
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- You’re unconditionally setting SUPPORT_EXPORT_IDS for every InnerIndexInterface in its constructor, which can advertise export support even when no specialized implementation exists; consider moving feature registration into only those index types that truly support ExportIDs.
- The ExportIDs implementation eagerly allocates and copies all labels, which could be expensive for large datasets; consider providing a zero-copy view or sharing the underlying label buffer when safe.
- ExportIDs falls back to throwing std::runtime_error on unsupported indices, but other methods use tl::expected<...,Error>; consider aligning on the expected-based error handling to keep the API consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’re unconditionally setting SUPPORT_EXPORT_IDS for every InnerIndexInterface in its constructor, which can advertise export support even when no specialized implementation exists; consider moving feature registration into only those index types that truly support ExportIDs.
- The ExportIDs implementation eagerly allocates and copies all labels, which could be expensive for large datasets; consider providing a zero-copy view or sharing the underlying label buffer when safe.
- ExportIDs falls back to throwing std::runtime_error on unsupported indices, but other methods use tl::expected<...,Error>; consider aligning on the expected-based error handling to keep the API consistent.
## Individual Comments
### Comment 1
<location> `src/label_table.h:115` </location>
<code_context>
return this->label_table_[inner_id];
}
+ inline const LabelType*
+ GetAllLabels() const {
+ return label_table_.data();
+ }
+
</code_context>
<issue_to_address>
Returning raw pointer exposes internal storage.
This approach risks dangling pointers if the vector is modified. Please document this behavior or restrict usage to prevent misuse.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
inline const LabelType*
GetAllLabels() const {
return label_table_.data();
}
=======
// WARNING: Returns a pointer to the internal storage of label_table_.
// The returned pointer may become invalid if label_table_ is modified (e.g., resized).
// Use only when you can guarantee that label_table_ will not be changed while the pointer is in use.
// This function is intended for advanced usage only.
inline const LabelType*
GetAllLabels() const {
return label_table_.data();
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `include/vsag/index.h:541` </location>
<code_context>
+ * @return DatasetPtr A pointer to the exported IDs dataset.
+ * @throws std::runtime_error If the index does not support exporting the IDs.
+ */
+ virtual tl::expected<DatasetPtr, Error>
+ ExportIDs() const {
+ throw std::runtime_error("Index doesn't support ExportIDs");
+ }
+
</code_context>
<issue_to_address>
Throwing runtime_error may not match expected error handling.
Consider returning tl::unexpected(Error(...)) instead of throwing, to align with the method's error handling approach.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1069 +/- ##
==========================================
+ Coverage 91.67% 91.74% +0.07%
==========================================
Files 303 303
Lines 17278 17295 +17
==========================================
+ Hits 15839 15867 +28
+ Misses 1439 1428 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
LHT129
added a commit
to LHT129/vsag
that referenced
this pull request
Apr 16, 2026
Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
LHT129
added a commit
that referenced
this pull request
May 11, 2026
Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
Sia-Sheerland
pushed a commit
to Sia-Sheerland/vsag
that referenced
this pull request
Jun 26, 2026
Signed-off-by: LHT129 <tianlan.lht@antgroup.com> Signed-off-by: Sia Sheerland <x1075956441x@163.com> Signed-off-by: Sia Sheerland <x1075956441x@163.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closed: #1068
Summary by Sourcery
Add support for exporting all IDs from an index through a new ExportIDs API, enable it via a feature flag, implement it in index internals, and validate it with unit tests.
New Features:
Enhancements:
Documentation:
Tests: