[Enhancement] Name shared-data vector index files by the segment's owning tablet id#75702
Merged
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0f8a18adb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
sevev
force-pushed
the
feat/sd-vi-owner-tablet-id
branch
from
c848776 to
e7db02e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7db02e821
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
sevev
force-pushed
the
feat/sd-vi-owner-tablet-id
branch
2 times, most recently
from
fd9414a to
e6f3a96
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 061af1e782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
xiangguangyxg
previously approved these changes
Jul 3, 2026
…ning tablet id Follow-up to StarRocks#75542, which added vector-index (.vi) support for shared-data bundle-file segments and named each .vi <segment_base>_<tablet_id>_<index_id>.vi using the id of the tablet touching the segment. That id is not reader-agnostic: after a tablet split a segment is shared by multiple child tablets (SegmentMetadataPB.shared, carried verbatim via CopyFrom), and each child derives a different .vi name than the writer produced. On read this yields NotFound and a silent fall back to brute-force search; in GC/vacuum the real .vi is neither protected nor collected under the name every referencing tablet computes. Record the writing tablet's id in the new persisted field SegmentMetadataPB.vector_index_tablet_id (carried verbatim across split via CopyFrom) and derive every .vi name from that recorded owner instead of the local tablet id, falling back to the current tablet id for segments written before the field existed. The .vi filename format is unchanged; only the source of the embedded id changes. The owner id is stamped at write time (normal write, partial-update rewrite, deferred async build), threaded through FileInfo / SegmentReadOptions, and consumed by the read path, the async build task, and GC. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: sevev <qiangzh95@gmail.com>
Address review: VerticalGeneralTabletWriter::finish records vector_index_ids directly rather than through record_segment_vector_index_ids, so it never stamped vector_index_tablet_id; VerticalPkTabletWriter inherits that finish and had the same gap. Set the owner id there too (the .vi is already named with _tablet_id by fill_vector_index_file_paths, so the owner is _tablet_id), and assert vector_index_tablet_id in the horizontal / horizontal-pk / vertical tablet-writer tests. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review: rewrite_segment recorded vector_index_tablet_id only in the copy-only async partial-update branch. Auto-increment rewrites and async partial updates with vector-indexed unmodified columns also leave vector_index_ids in file_info (via SegmentRewriter) but left the owner as -1, so MetaFileBuilder persisted the ids with the owner cleared. A later tablet split would then derive <segment>_<child_id>_<idx>.vi instead of the file written under the writer id. Centralize the stamp in stamp_rewrite_vector_index_owner and call it before every replace-segment assignment (the dest is always written by this tablet, so the .vi owner is params.tablet->id()). Assert vector_index_tablet_id on the persisted metadata across all rewrite-path tests in LakePartialUpdateVectorIndexTest. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e resolve helper Write side: fill_missing_vector_index_owner() at every publish funnel (PK apply_opwrite / set_final_rowset / apply_opcompaction; non-PK write / compaction / schema-change apply) stamps the owner when a writer path recorded vector_index_ids without it. Segments marked shared() (split cross-publish, written by another tablet) and replication are deliberately skipped. Read side: collapse the ten owner-or-fallback ternaries into resolve_vector_index_owner_tablet_id / gen_vector_index_filename_for_segment in filenames.h, so the fallback rule lives in one place. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gen_vector_index_filename / gen_vector_index_path_from_segment_path are plain string formatters and carry no owner semantics; restore their original tablet_id parameter and comments. The owner rule lives in resolve_vector_index_owner_tablet_id and the SegmentMetadataPB field comment. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Concentrate the write-side logic: writers no longer carry or set vector_index_tablet_id (drop the FileInfo field, the writer/rewrite stamps and the to_proto copy). fill_missing_vector_index_owner at the publish funnels becomes the sole producer — there the publishing tablet is the writing tablet for every non-shared segment, so the stamped value always matches the id embedded in the .vi filenames on disk. Consumers are unchanged. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Writers record vector_index_tablet_id into FileInfo alongside vector_index_ids (tablet writers and stamp_rewrite_vector_index_owner for segment rewrites); to_proto persists it and the publish apply carries it verbatim — the apply-funnel stamping is removed and txn_log_applier reverts to main. Carry the owner through VectorIndexBuildTask via FileInfo so build_segment keeps its original signature; fill SegmentReadOptions only in Rowset::read(), the sole entry point serving ANN queries. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A split-shared segment's .vi is named by the recorded owner, so it is the same file every sibling tablet references. Give it the same shared-file lifecycle as its segment: route deletion through the shared-file deleter (or retain when the segment is retained) in delete_rowset_vi_files; delay-delete alive shared .vi in collect_alive_shared_files; and carry src_seg_meta.shared() onto the orphaned source .vi in rewrite_segment. A bundle-only segment's .vi stays per-tablet (the filename embeds the writer id) and keeps ordinary deletion. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ded owner Every persisted vector-indexed segment records its owner at write time (the tablet writers and the segment rewrite stamp it; ADD VECTOR INDEX rewrites through the tablet writer, never the no-rewrite fast path), so the shared-data vector index has no owner-less on-disk state. Drop the "recorded owner or local tablet id" fallback: vector_index_owner_tablet_id(segment_meta) returns the recorded owner and DCHECKs its presence, and its callers (read / GC / build / rewrite) lose the fallback_tablet_id argument; segment_iterator DCHECKs the option is set instead of defaulting to the reading tablet. A future write path that forgets to stamp now fails loudly in tests rather than silently degrading to brute-force reads and mis-named GC. Test metadata that modeled the old fallback (vector_index_ids without an owner) now records the owner, matching the .vi files each test creates. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…allback invariant The final design DCHECKs the recorded owner instead of falling back to the reading tablet id, but the SegmentMetadataPB.vector_index_tablet_id comment still described the removed fallback. Reword it: the owner is always stamped in the same step that records vector_index_ids, and consumers (read / GC / async build) DCHECK its presence for vector-indexed segments with no fallback. Signed-off-by: sevev <qiangzh95@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The vector-index build tests hand-build TabletMetadataPB but omitted vector_index_tablet_id, which the production write path always stamps alongside vector_index_ids. Under release builds the guarding DCHECK in vector_index_owner_tablet_id() is compiled out, so the build named the .vi with owner=0 while the assertions expected owner=kTabletId, failing LakeServiceVectorIndexBuildTest.test_build_vector_index_full_path and test_build_vector_index_partial_failure. Set vector_index_tablet_id in the test metadata to mirror the write path. Signed-off-by: sevev <qiangzh95@gmail.com>
vector_index_ids and vector_index_tablet_id are per-segment attributes but sat on the generic FileInfo, and SegmentFileInfo already shadowed vector_index_ids -- a latent footgun where writes and reads could target different copies. Move both fields down to SegmentFileInfo (the only copy now) and thread SegmentFileInfo through the lake rewrite/apply/build-task paths and the replace_segments maps. The shared SegmentRewriter keeps its FileInfo* dest (still used by the shared-nothing storage engine) and instead returns the recorded ids through a new optional out_vector_index_ids parameter, so no downcast and no behavior change for either engine. Signed-off-by: sevev <qiangzh95@gmail.com>
sevev
force-pushed
the
feat/sd-vi-owner-tablet-id
branch
from
c3ff3ac to
e262cc5
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e262cc57f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
After rebasing onto StarRocks#75588 (which added shared-.vi vacuum protection using the old tablet-id-based .vi name), the owner-based .vi handling only ran for segment_meta.shared(), missing bundled split segments (is_shared_segment but not shared()). A parent/sibling vacuum could then delete the owner-named .vi while a child still referenced the bundled segment, dropping its index. Make .vi handling mirror the segment it belongs to in every vacuum path: - collect_alive_shared_files: delay-delete the owner-named .vi for every is_shared_segment (bundle or shared), not just shared(); drop the stale old-name delay and the redundant shared()-only block. - split-metadata GC: delete the owner-named .vi under the same !is_shared_segment || allow_delete_shared_files guard as the segment, and drop the duplicate delete_rowset_vi_files call that used a different (refcount) retention policy than the segment's skip-to-retain. Signed-off-by: sevev <qiangzh95@gmail.com>
The field/param named ...tablet_id read as "the current tablet", but its role is a per-segment unique key that disambiguates a segment's .vi filenames (so bundle-file segments shared across tablets get distinct .vi, and the name stays reader-agnostic across tablet split). Its value happens to be the writing tablet's id, used purely as a unique key. Name it by that role, value-agnostically: - SegmentMetadataPB.vector_index_tablet_id -> segment_vector_index_uid (field ordinal 13 kept -> wire compatible) - SegmentFileInfo / SegmentReadOptions members likewise - helper vector_index_owner_tablet_id() -> resolve_segment_vector_index_uid() (distinct from the raw getter segment_vector_index_uid()) - reword comments to lead with the role; note the value is the writer id The low-level gen_vector_index_filename(..., tablet_id, ...) formatter param is left unchanged (it is a generic string formatter with no owner semantics). Pure rename, no behavior change. Signed-off-by: sevev <qiangzh95@gmail.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
luohaha
approved these changes
Jul 3, 2026
Contributor
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Contributor
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
Contributor
[BE Incremental Coverage Report]✅ pass : 54 / 60 (90.00%) file detail
|
xiangguangyxg
approved these changes
Jul 3, 2026
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
5 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.
Why I'm doing:
Follow-up to #75542, which added vector-index (
.vi) support for shared-data bundle-filesegments. That PR names each
.vi<segment_base>_<tablet_id>_<index_id>.vi, embedding thetablet id to disambiguate segments that share one physical bundle file across tablets. It uses
the id of the tablet touching the segment, which is not reader-agnostic.
After a tablet split, a single segment is shared by multiple child tablets
(
SegmentMetadataPB.shared, carried verbatim viaCopyFrom). Each child reads the sharedsegment with its own tablet id, so it derives a different
.viname than the one the writeractually produced:
..._<child_id>_<index_id>.vi, which does not exist → NotFound→ silent fall back to brute-force vector search (results stay correct, but slow).
tablet id, so the real
.vi(named after the writer) is neither protected nor collected underthe name every referencing tablet computes — it can be deleted while still referenced, or
leaked.
What I'm doing:
Record the writing tablet's id in a new persisted field
SegmentMetadataPB.vector_index_tablet_idand derive every.viname from that recorded ownerinstead of the local tablet id. Because the field is carried verbatim across split via
CopyFrom, every child that shares the segment resolves the exact same.vithe writer wrote..vifilename format is unchanged (<segment_base>_<owner_tablet_id>_<index_id>.vi);only the source of the embedded id changes from "the tablet touching the segment" to "the
recorded owner".
vector_index_ids: thetablet writers (normal write and the deferred-async initial write) and the segment rewrite
(partial-update / auto-increment).
to_protopersists it and the publish apply carries itverbatim. From there it is threaded through
FileInfo/SegmentReadOptionsand consumed bythe read path (
segment_iterator), the async build (vector_index_build_task), and GC(
vacuum/transactions).vector_index_idsalways records this ownertoo, and every consumer reads the owner only for a vector-indexed segment and
DCHECKs itspresence — there is no fall back to the reading tablet id. A future write path that records ids
but forgets the owner fails loudly in tests instead of silently degrading to brute-force reads
and mis-named GC. This PR is a follow-up to [Enhancement] Build vector index for shared-data bundle segments #75542 (unreleased) and ships together with it, so
no released version ever persisted a vector-indexed segment without the owner.
.viis named by the recorded owner, so it is one physical file referencedby every sibling tablet. It follows its segment's shared-file lifecycle in GC: routed through
the shared-file deleter / retained while any sibling still references it, and delay-deleted like
the shared segment. A bundle-only
.vistays per-tablet (the filename embeds the writer id)and keeps ordinary per-tablet deletion.
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: