refactor(ivf): split implementation by responsibility

[LHT129]

Summary

• split the 2,691-line ivf.cpp implementation into responsibility-focused translation units
• follow the existing HGraph organization with build, search, serialize, and modify files
• keep ivf.h and all behavior unchanged
• register the new source files in the IVF CMake object target

Closes: #2803

File organization

• ivf.cpp: core construction, shared helpers, merge, model export, distance, statistics
• ivf_build.cpp: initialization, build/train/add, bucket graph construction and rebuild
• ivf_search.cpp: KNN/range/request search, routing, reorder, reasoning report
• ivf_serialize.cpp: serialization/deserialization and streaming helpers
• ivf_modify.cpp: remove and attribute updates

Validation

• make fmt (clang-format 15)
• Release build (libvsag.so and libvsag_static.a)
• focused IVF PR regression suite: 43 test cases, 403,551 assertions
• clang-tidy 15 on all five IVF translation units
• git diff --check

[vsag-bot]

/label status/ready-to-merge
/waiting-on maintainer
/request-review @jiaweizone
/request-review @inabao

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[mergify]

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

• label~=^kind/

🟢 Require version label

• label~=^version/

[LHT129]

Summary: This is a clean refactor that splits ivf.cpp (2691 lines) into 5 responsibility-focused translation units: ivf.cpp (core), ivf_build.cpp, ivf_search.cpp, ivf_serialize.cpp, ivf_modify.cpp. The code movement is correct — all functions are verbatim copies with no behavioral changes. INVALID_BUCKET_ID was properly scoped to file-local static constexpr in ivf_search.cpp.

Main observation: All 4 new files inherited an overly broad #include list from the original ivf.cpp. Each file includes ~40 headers but only needs a fraction of them. Consider trimming each file to only the includes actually required — this reduces unnecessary recompilation and makes dependencies explicit. This is non-blocking and can be addressed in a follow-up.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[LHT129]

After thorough review of commit 41e6a5b, this is a clean, well-executed refactoring. The previous concerns about overly broad includes in the split files have been addressed — each new translation unit now includes only the headers it actually needs.

Key observations:

• ivf.cpp (840 lines): Core construction, shared helpers (merge, model export, distance, statistics, memory usage)
• ivf_build.cpp (468 lines): InitFeatures, Build, Train, Add, build_bucket_graphs, fill_location_map, RebuildBucketGraphs
• ivf_search.cpp (817 lines): KnnSearch, RangeSearch, SearchWithRequest, search, reorder, route_buckets_only
• ivf_serialize.cpp (661 lines): Serialize, Deserialize, streaming serialization helpers
• ivf_modify.cpp (48 lines): Remove, UpdateAttribute (two overloads)

No correctness issues found. The function distribution is logical, all cross-file member function calls resolve correctly via the linker, and the exception safety in RebuildBucketGraphs is properly handled. The CMakeLists.txt correctly registers all 4 new source files.

One minor note: the // TODO(LHT): duplicate comments in Build() and Add() (ivf_build.cpp lines 111, 148) are pre-existing and not introduced by this refactoring, but worth addressing in a follow-up.

[LHT129]

The include-cleanup review suggestions have been addressed in 41e6a5b5, and each inline comment has been replied to. All required CI checks are now green (ASan/TSAN, compatibility, unit tests, functional tests, examples, CircleCI, DCO, and formatting).

The PR remains a single commit, is conflict-free, and is ready for maintainer approval/merge. Review requested from @jiaweizone, @wxyucs, and @inabao.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[LHT129]

Continuing the include-cleanup work from 41e6a5b — ivf.cpp itself still has ~15 includes that are no longer needed after the split.

[suggestion] The following #include directives in ivf.cpp can be removed since their symbols are only used in code that was moved to ivf_build.cpp, ivf_search.cpp, or ivf_serialize.cpp:

Include	Reason
attr/argparse.h	AstParse → ivf_search.cpp
attr/executor/executor.h	Executor → ivf_search.cpp
datacell/graph_datacell_parameter.h	unused in remaining code
datacell/graph_interface_parameter.h	unused in remaining code
impl/heap/standard_heap.h	StandardHeap → ivf_search.cpp
impl/inner_search_param.h	InnerSearchParam → ivf_search.cpp
impl/pruning_strategy.h	select_edges_by_heuristic → ivf_build.cpp
impl/reasoning/search_reasoning.h	ReasoningContext → ivf_search.cpp
impl/searcher/basic_searcher.h	BasicSearcher → ivf_search.cpp
index_feature_list.h	IndexFeatureList → ivf_build.cpp
io/reader_io/reader_io_parameter.h	ReaderIOParameter → unused
query_context.h	QueryContext → ivf_search.cpp (already transitively available via ivf.h)
simd/normalize.h	Normalize → ivf_search.cpp
storage/serialization.h	Metadata/Footer → ivf_serialize.cpp
storage/serialization_tags.h	StreamSerializationTag → ivf_serialize.cpp
storage/stream_reader.h	StreamReader → ivf_serialize.cpp
storage/tlv_section.h	WriteStreamingBlock → ivf_serialize.cpp
utils/search_threshold.h	ParseSearchThreshold → ivf_search.cpp
utils/visited_list.h	VisitedList → ivf_search.cpp

Note: inner_string_params.h and storage/stream_writer.h are still directly used (string constants and IOStreamWriter).

[LHT129]

Addressed the latest core include-cleanup suggestion in 75727617: removed the 19 headers no longer directly used by ivf.cpp after the split. I also added explicit <fmt/format.h> dependencies to all three IVF translation units that call fmt::format (ivf.cpp, ivf_search.cpp, and ivf_serialize.cpp).

Revalidated clang-format 15, Release build, clang-tidy 15, focused IVF PR tests (43 cases), single-commit history, and latest-main rebase.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[LHT129]

After thorough review of commit 7572761, this is a clean, well-executed refactoring. The split of the 2,691-line ivf.cpp into 5 responsibility-focused translation units follows the established HGraph organization pattern and keeps all behavior unchanged.

What was checked:

• All 6 changed files reviewed in full (CMakeLists.txt, ivf.cpp, ivf_build.cpp, ivf_search.cpp, ivf_serialize.cpp, ivf_modify.cpp)
• ivf.h is unchanged as claimed
• CMakeLists.txt correctly registers all 4 new source files
• The #include <fmt/format.h> fix from the previous Copilot review is properly applied to both ivf_search.cpp and ivf_serialize.cpp
• No logic changes — pure code movement between translation units
• All includes in the new split files are appropriate for their respective responsibilities

No blocking issues found. The refactoring is well-structured and improves maintainability by separating build, search, serialize, and modify concerns into dedicated files.

[wxyucs]

lgtm

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request