[Backport 1.0] feat(pyramid): support paths in GetDataByIdsWithFlag

[jac0626]

Change Type

• Improvement

Linked Issue

Related to #2754. Backport of #2763 to the 1.0 branch.

What Changed

• Add store_paths as an opt-in Pyramid parameter, disabled by default.
• Return retained paths only when GetDataByIdsWithFlag selects DATA_FLAG_PATH; named hierarchies use Dataset::GetPaths(name).
• Keep storage isolated in PyramidPathStore with only direct dense uint64 offsets, uint16 counts, and a flat string vector. Rows support 0/1/N paths without a map, sparse/adaptive policy, magic, private version, or row-state protocol.
• Record successfully allocated inner IDs for NSW, ODescent, and Add.
• Serialize the three fields directly through existing StreamWriter primitives; regular serialization is parameter-driven and streaming uses PYRAMID_PATHS.

Compatibility and Performance

• Default-off behavior allocates no per-vector path state and writes no path sidecar.
• The in-memory writer is identical to main. Main Release A/B against the prior single-path implementation measured build +1.27% / +1.86% and peak build RSS +1.16 / +1.43 MiB at 1K / 100K unique paths for 100K vectors.
• Direct serialization was 1.90% / 0.46% faster than the prior unmerged row-state draft and adds 900,016 serialized bytes at 100K slots.

Validation

• Debug unittests and functests targets built successfully.
• Pyramid unit tests: 570 assertions / 11 cases.
• Pyramid path functional tests: 280 assertions / 16 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.

[vsag-bot]

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

[LHT129]

Reviewed the latest commit (523f0f8). All issues raised in the previous review round have been addressed:

• Double label lookup: Resolved by the get_data_by_ids / get_data_by_ids_with_flag refactoring in inner_index_interface.cpp, which resolves inner IDs once and reuses them for vector, attribute, extra-info, and now path retrieval.
• Circular include: Resolved by moving Pyramid::serialize_paths, deserialize_paths, and GetDataByIds into the dedicated pyramid_paths.cpp file. pyramid_path_store.cpp no longer includes pyramid.h.
• Serialization tag criticality: PYRAMID_PATHS is correctly marked as non-critical in serialization_tags.h with a comment explaining the rationale.
• Deserialization contract: The Footer::Parse modernization is consistent with other index types (hgraph, ivf, sindi_v2). The has_paths != store_paths_ mismatch check and format version validation are good safety measures.

The new code is well-structured, properly tested (unit tests for path store, functional tests for paths and serialization, parameter tests), and follows the project coding standards. No new issues found.

[LHT129]

[note] This is a well-structured backport of the Pyramid path storage feature (#2763) to the 1.0 branch. The code quality is high, with thorough test coverage, proper serialization compatibility checks, and clean separation of concerns between PyramidPathStore and the Pyramid class.

One minor observation in pyramid_paths.cpp GetDataByIdsWithFlag: the paths unique_ptr is allocated for every hierarchy before checking whether GetPaths succeeds. When a hierarchy lacks complete paths, the allocation is discarded. Since hierarchies_.size() is typically small (1-3) and path retrieval is not a hot path, this is acceptable. Consider pre-checking completeness before allocating if this becomes a concern in the future.

All previous review comments from LHT129 have been addressed:

• Redundant label lookup: resolved via get_data_by_ids_with_flag with shared inner_ids
• Circular include: resolved by separating Pyramid methods into pyramid_paths.cpp
• PYRAMID_PATHS tag numbering: clarified with a comment about non-critical classification
• Deserialize footer change: confirmed behaviorally equivalent to the old read_index_footer

[jac0626]

Updated in a5da8efd with the same low-coupling write boundary as main:

• PyramidPathStore no longer receives input offsets, a first ID, or Build/Add semantics.
• Pyramid retains each actually allocated inner_id; the store-owned Writer receives only (inner_id, path).
• One Writer lock covers the batch, avoiding the per-path locking regression while remaining safe for concurrent Add.
• Writer is non-copyable/non-movable and dual-vector growth restores the previous size on allocation failure.
• Added NSW/ODescent, middle-duplicate, out-of-order/hole, and concurrent Add mapping coverage.

The main-branch Release A/B for this Writer design was +1.09% at 1K unique paths and +0.14% at 100K unique paths versus the previous bulk implementation, with less than 0.8 MiB RSS difference and identical serialized sizes.

Local targeted result: 18 PathStore unit assertions + 280 Pyramid path functional assertions passed; clang-format-15 and git diff --check passed.

[LHT129]

[note] In Deserialize (src/algorithm/pyramid/pyramid.cpp), body_length is computed as reader.Length() - footer->Length(). Both are uint64_t, so if footer->Length() exceeded reader.Length() (which should never happen after a successful Footer::Parse), this would underflow. Consider adding CHECK_ARGUMENT(footer->Length() <= reader.Length(), "footer exceeds reader length") to make the invariant explicit and protect against future changes to Footer::Parse.

Overall assessment: This is a well-structured backport PR with comprehensive test coverage (15 functional test cases, 4 unit test cases, plus parameter and serialization tests). The implementation correctly isolates path storage in PyramidPathStore, properly handles concurrent Add operations, validates serialization contract consistency, and preserves backward compatibility when store_paths=false. All previously identified issues have been addressed. No blocking issues found.

[jac0626]

Final minimal-scope update is pushed in 0bd344d.

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.
• 1.0 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: 570 assertions / 11 cases
• Path FT: 280 assertions / 16 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.

The main-branch Release A/B remains representative because the in-memory writer is identical: versus the prior single-path implementation, build time was +1.27% at 1K unique paths and +1.86% at 100K unique paths; peak build RSS was +1.16 MiB and +1.43 MiB. Direct serialization was 1.90% and 0.46% faster than the prior unmerged row-state draft, while the direct dense wire format adds 900,016 bytes at 100K slots.

[LHT129]

[note] The deserialize_paths method does not check for duplicate hierarchy names in the serialized stream. If a malformed/corrupted serialized payload contains the same hierarchy name twice, the second occurrence would silently overwrite the first deserialized path store without any error. The previous version of this code (before the refactor) used an UnorderedSet to guard against duplicates. Consider adding a similar duplicate-detection check here to fail fast on corrupted data rather than silently accepting it.

[LHT129]

PR Review Summary

This PR backports store_paths support to Pyramid, enabling GetDataByIdsWithFlag to return original path labels. The implementation is well-structured and follows existing patterns in the codebase.

What changed

• New PyramidPathStore class with RAII Writer pattern for thread-safe path storage
• New pyramid_paths.cpp with serialization and GetDataByIdsWithFlag
• store_paths parameter (default false) added to PyramidParameters
• Streaming serialization tag PYRAMID_PATHS = 18
• InnerIndexInterface refactored to expose get_data_by_ids_with_flag as protected
• Comprehensive test coverage: pyramid_path_store_test, test_pyramid_path_serialization, test_pyramid_paths

Review findings

1. [note] deserialize_paths missing duplicate hierarchy name check (see inline comment on pyramid_paths.cpp:47): The method does not guard against duplicate hierarchy names in corrupted serialized data. The previous version used an UnorderedSet for this check.

Overall assessment

The implementation is solid: correct use of RAII, proper error handling, good test coverage, and clean integration with existing serialization infrastructure. No blocking issues found.