feat(pyramid): support paths in GetDataByIdsWithFlag

[jac0626]

Change Type

• Bug fix
• New feature
• Improvement/Refactor
• Documentation
• CI/Build/Infra

Linked Issue

• Resolves: [improve](pyramid): GetDataByIds support paths of pyramid #2754

What Changed

• Add the opt-in Pyramid parameter store_paths, disabled by default.
• Return retained paths only from GetDataByIdsWithFlag when DATA_FLAG_PATH is selected; plain GetDataByIds keeps its existing result.
• Keep storage isolated in PyramidPathStore. Each hierarchy uses direct dense SoA fields: uint64 offsets, uint16 counts, and a flat string vector, supporting 0/1/N paths without a map, sparse/adaptive policy, or path-tree metadata.
• Record only successfully allocated inner IDs across NSW, ODescent, Build Cache, and Add.
• Serialize the three fields directly with existing StreamWriter primitives. Regular serialization is parameter-driven; streaming uses the generic PYRAMID_PATHS TLV block.
• Cover default-off behavior, Build/Add/Clone/concurrent Add, named and incomplete hierarchies, regular/streaming serialization, and Build Cache.

Test Evidence

• Debug unittests and functests targets built successfully.
• Pyramid unit tests: 207 assertions / 19 cases.
• Pyramid path functional tests: 271 assertions / 15 cases.
• clang-format-15, clang-tidy-15 on changed implementation files, and git diff --check passed.
• Independent final review found no P0-P2 issue and no remaining out-of-scope or over-defensive design.

Compatibility Impact

• API/ABI: additive store_paths parameter and DATA_FLAG_PATH constant.
• Default behavior: unchanged; no data-sized path storage and no path sidecar when disabled.
• Enabled serialization requires a reader configured with store_paths=true and its matching path payload.

Performance and Concurrency Impact

Release A/B with 100K vectors, dim=4, NSW, store_paths=true, 7 alternating runs against the prior single-path implementation:

• Build: +1.27% at 1K unique paths; +1.86% at 100K unique paths.
• Peak build RSS: +1.16 MiB and +1.43 MiB respectively.
• Direct serialization versus the prior unmerged row-state draft: 1.90% and 0.46% faster.
• Direct dense serialized fields add 900,016 bytes at 100K slots.

Each enabled hierarchy has one shared mutex; one Writer holds the exclusive lock for a batch. Default-off allocates no per-vector path state.

Documentation Impact

English and Chinese Pyramid, index-parameter, and API docs are updated.

Risk and Rollback

• Risk level: medium.
• Rollback: revert the PR; default-off serialization is sidecar-free.

Checklist

• Linked issue
• Tests added/updated
• Compatibility reviewed
• Documentation updated
• Conventional commits and DCO trailers

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao

[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/

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR adds an opt-in Pyramid build parameter (store_paths) that retains original per-ID paths (default and named hierarchies) and returns them from GetDataByIds, including persistence through regular and streaming serialization.

Changes:

• Introduces store_paths parameter plumbing (parsing, JSON, compatibility checks, external param mapping).
• Implements PyramidPathStore with per-hierarchy path retention and incorporates it into Build/Add/Clone and GetDataByIds.
• Adds serialization support (BinarySet + streaming) and expands unit/functional tests and docs (EN/ZH).

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/test_pyramid_paths.cpp	Adds functional tests for default/named hierarchy path return behavior.
tests/test_pyramid_path_serialization.cpp	Adds tests for BinarySet + streaming serialization of stored paths.
tests/test_pyramid.cpp	Extends cache import/export smoke test to assert paths survive warmed build.
src/storage/serialization_tags.h	Adds PYRAMID_PATHS streaming tag and mapping helpers.
src/inner_string_params.h	Adds internal key string PYRAMID_STORE_PATHS_KEY.
src/constants.cpp	Adds exported constant PYRAMID_STORE_PATHS.
src/algorithm/pyramid/pyramid_zparameters_test.cpp	Adds unit tests for store_paths param + compatibility, updates mapping test.
src/algorithm/pyramid/pyramid_zparameters.h	Adds store_paths field to PyramidParameters.
src/algorithm/pyramid/pyramid_zparameters.cpp	Implements parse/emit/compatibility for store_paths.
src/algorithm/pyramid/pyramid_path_store_test.cpp	Adds unit tests for PyramidPathStore behavior + serialization robustness.
src/algorithm/pyramid/pyramid_path_store.h	Introduces PyramidPathStore API.
src/algorithm/pyramid/pyramid_path_store.cpp	Implements PyramidPathStore and adds Pyramid path (de)serialization + GetDataByIds path attachment.
src/algorithm/pyramid/pyramid.h	Wires store_paths_ + per-hierarchy path_store, declares path helpers and overrides GetDataByIds.
src/algorithm/pyramid/pyramid.cpp	Records paths on build/add, persists paths in regular + streaming serialization, adds external param mapping.
src/algorithm/pyramid/CMakeLists.txt	Builds new pyramid_path_store.cpp compilation unit.
include/vsag/constants.h	Exposes PYRAMID_STORE_PATHS constant.
docs/docs/zh/src/resources/index_parameters.md	Documents store_paths in zh index-parameter resources.
docs/docs/zh/src/indexes/pyramid.md	Documents path retrieval and serialization semantics in zh Pyramid docs.
docs/docs/en/src/resources/index_parameters.md	Documents store_paths in en index-parameter resources.
docs/docs/en/src/indexes/pyramid.md	Documents path retrieval and serialization semantics in en Pyramid docs.

Suppressed comments (1)

src/storage/serialization_tags.h:119

• For streaming serialization, PYRAMID_PATHS is emitted only when store_paths_ is enabled, and the loader enforces the block’s presence in that configuration—so the block is effectively required when present. Marking it non-critical means older readers that don’t understand PYRAMID_PATHS may silently skip it and still “successfully” load the index (but lose the promised path semantics), which is risky operationally. Consider making this block critical when store_paths is enabled (e.g., by treating PYRAMID_PATHS as critical in StreamSerializationTagCritical, or by overriding the critical flag passed to AppendStreamingManifestBlock / WriteStreamingBlock for this tag when store_paths_ is true).

StreamSerializationTagCritical(uint32_t tag) {
    switch (static_cast<StreamSerializationTag>(tag)) {
        case StreamSerializationTag::SECTION_END:
            return false;
        case StreamSerializationTag::LABEL_TABLE:
        case StreamSerializationTag::BASE_CODES:
        case StreamSerializationTag::BOTTOM_GRAPH:
        case StreamSerializationTag::ROUTE_GRAPHS:
        case StreamSerializationTag::IVF_BUCKET:
        case StreamSerializationTag::IVF_PARTITION_STRATEGY:
        case StreamSerializationTag::HIGH_PRECISION_CODES:
        case StreamSerializationTag::SINDI_WINDOWS:
        case StreamSerializationTag::SINDI_RERANK_INDEX:
        case StreamSerializationTag::SINDI_TERM_ID_MAPPER:
        case StreamSerializationTag::PYRAMID_HIERARCHIES:
        case StreamSerializationTag::CODE_SLOT_MAP:
        case StreamSerializationTag::IVF_PRECISE_BUCKET:
            return true;
        case StreamSerializationTag::ATTRIBUTE_FILTER:
        case StreamSerializationTag::EXTRA_INFO:
        case StreamSerializationTag::RAW_VECTOR:
        case StreamSerializationTag::IVF_BUCKET_GRAPH:
        case StreamSerializationTag::PYRAMID_PATHS:
            return false;
    }
    return false;
}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Suppressed comments (1)

src/storage/serialization_tags.h:119

• PYRAMID_PATHS is treated as non-critical here, but when store_paths_ is enabled the streaming loader hard-requires the paths block and fails if it's missing. Marking it non-critical can allow older readers (that don't understand the tag) to silently ignore the block and potentially load an index without its required payload. Consider making PYRAMID_PATHS critical (or at least setting the manifest's critical flag to true when store_paths_ is enabled) to better reflect the 'required when enabled' contract and improve forward/backward behavior.

StreamSerializationTagCritical(uint32_t tag) {
    switch (static_cast<StreamSerializationTag>(tag)) {
        case StreamSerializationTag::SECTION_END:
            return false;
        case StreamSerializationTag::LABEL_TABLE:
        case StreamSerializationTag::BASE_CODES:
        case StreamSerializationTag::BOTTOM_GRAPH:
        case StreamSerializationTag::ROUTE_GRAPHS:
        case StreamSerializationTag::IVF_BUCKET:
        case StreamSerializationTag::IVF_PARTITION_STRATEGY:
        case StreamSerializationTag::HIGH_PRECISION_CODES:
        case StreamSerializationTag::SINDI_WINDOWS:
        case StreamSerializationTag::SINDI_RERANK_INDEX:
        case StreamSerializationTag::SINDI_TERM_ID_MAPPER:
        case StreamSerializationTag::PYRAMID_HIERARCHIES:
        case StreamSerializationTag::CODE_SLOT_MAP:
        case StreamSerializationTag::IVF_PRECISE_BUCKET:
            return true;
        case StreamSerializationTag::ATTRIBUTE_FILTER:
        case StreamSerializationTag::EXTRA_INFO:
        case StreamSerializationTag::RAW_VECTOR:
        case StreamSerializationTag::IVF_BUCKET_GRAPH:
        case StreamSerializationTag::PYRAMID_PATHS:
            return false;
    }
    return false;
}

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/algorithm/inner_index_interface.cpp:842

• get_data_by_ids_with_flag divides by build_thread_count_ without guarding against it being 0. InnerIndexParameter::FromJson accepts "build_thread_count": 0, which would make item_per_thread = (count + thread_count - 1) / thread_count a division-by-zero UB when GetDataByIdsWithFlag is called. Clamp the computed thread count to at least 1 before calculating item_per_thread.

    auto thread_count = static_cast<int64_t>(this->build_thread_count_);
    auto item_per_thread = (count + thread_count - 1) / thread_count;
    auto dataset = Dataset::Make();

[jac0626]

Final minimal-scope update is pushed in 087ff82.

Implementation:

• Path retention remains opt-in through store_paths=false by default; DATA_FLAG_PATH is required to return paths.
• PathStore is a separate component and accepts only allocated inner IDs plus paths.
• main uses only the direct dense SoA fields: Vector<uint64_t> offsets, Vector<uint16_t> counts, and a flat Vector paths.
• Rows support 0/1/N paths for the later feat(pyramid): support multiple paths per vector #2796 integration. There is no map, sparse/adaptive index, row-state protocol, magic, or private path format version.
• Serialization writes those three fields directly with the existing StreamWriter primitives.

Scope cleanup in this update:

• Removed the unused get_data_by_ids helper while preserving the original virtual GetDataByIds dispatch.
• Restored the existing footer/BufferStreamReader flow; only path reading is appended after the hierarchy data.
• Removed test-only Size APIs, duplicate full scans in GetPaths, temporary output copies, hierarchy bookkeeping beyond the existing pattern, and incomplete flat-pool total-count validation.
• Kept bounded length/range checks that prevent malformed input from causing unbounded allocation.

Validation:

• Pyramid UT: 207 assertions / 19 cases
• Path FT: 271 assertions / 15 cases
• clang-format-15, clang-tidy-15, build, and git diff --check passed
• Independent final review found no P0-P2 issue and no remaining out-of-scope or over-defensive design.

Release A/B, 100K vectors, dim=4, NSW, store_paths=true, 7 alternating runs:

• Build versus the prior single-path implementation: +1.27% with 1K unique paths; +1.86% with 100K unique paths.
• Peak build RSS: +1.16 MiB and +1.43 MiB respectively.
• Direct field serialization versus the prior unmerged row-state draft: 1.90% faster and 0.46% faster respectively.
• The direct dense wire format adds 900,016 bytes at 100K slots (9 bytes per slot plus field headers); this is the explicit cost of keeping offsets and uint16 counts directly serializable.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.

[LHT129]

Thank you for this well-structured PR. The separation of PyramidPathStore from pyramid_paths.cpp is clean, and the test coverage is thorough. I have a few observations:

[suggestion] pyramid_paths.cpp:91 — Eager path array allocation before validation

In GetDataByIdsWithFlag, for each hierarchy, std::make_unique<std::string[]>(count) is allocated before GetPaths validates the inner IDs. If GetPaths returns false (e.g. a hole or missing ID), the allocation is wasted. For large count and many hierarchies, this adds unnecessary peak memory. Consider calling GetPaths with a temporary vector first, or restructuring to allocate only after validation succeeds.

[suggestion] pyramid_path_store.cpp:141 — Unchecked data_biases[offset] index

In Writer::Insert, data_biases[offset] (int64_t) is used as an index into paths[...] without a non-negative check. While the callers (build_by_odescent, Add, build_with_cache) all populate data_biases from valid offsets, a defensive CHECK_ARGUMENT would guard against future misuse.

[note] pyramid.cpp:985 — Deserialize empty-index behavior

The Footer::Parse + EmptyIndex() check now throws INDEX_EMPTY for empty indexes. This is consistent with the previous read_index_footer behavior (which also threw on empty metadata), so no regression is introduced. The empty-index roundtrip tests in test_pyramid_path_serialization.cpp test Serialize/Deserialize on an index that was never built (0 elements), which succeeds because the index was never serialized with footer metadata — the Clone() path and the Serialize() of an empty index produce different codepaths than Deserialize of a previously-built-and-emptied index. This is worth documenting.

[note] serialization_tags.h:120 — Non-critical PYRAMID_PATHS tag

The PYRAMID_PATHS block is marked non-critical in the tag table, but read_streaming_body enforces its presence when store_paths_ is true. This is intentional: older readers that don't know about paths can skip the block, while readers configured with store_paths=true validate separately. The design is sound, though a brief comment in the tag table explaining this dual-enforcement pattern would help future maintainers.

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/algorithm/pyramid/pyramid_path_store.cpp:173

• Writer::Insert(...) always calls Prepare(...), even when the caller already reserved/expanded slots for a batch (e.g., Pyramid build/add paths). This adds avoidable per-insert checks and capacity computations in the hot path. Consider skipping Prepare when both the slot arrays and paths_ capacity are already sufficient, while still keeping the same argument validation.

    try {
        Prepare(slot + 1, path_count);
        for (uint64_t offset = 0; offset < path_count; ++offset) {

[LHT129]

LGTM

[mergify]

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

• Queue this pull request