Skip to content

feat: define pyramid named hierarchy API parameters#2104

Open
jac0626 wants to merge 5 commits into
antgroup:mainfrom
jac0626:codex/pyramid-hierarchy-api
Open

feat: define pyramid named hierarchy API parameters#2104
jac0626 wants to merge 5 commits into
antgroup:mainfrom
jac0626:codex/pyramid-hierarchy-api

Conversation

@jac0626
Copy link
Copy Markdown
Collaborator

@jac0626 jac0626 commented May 26, 2026

Change Type

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

Linked Issue

What Changed

  • Added named dataset path APIs: Dataset::Paths(name, paths) and Dataset::GetPaths(name).
  • Added Pyramid hierarchy parameter parsing for hierarchies, including per-hierarchy no_build_levels validation.
  • Added search parameter parsing for single hierarchy and reserved multi-hierarchy hierarchies + hierarchy_op selectors.
  • Added runtime guards so reserved multi-hierarchy execution fails clearly until the implementation PRs land.
  • Added unit coverage for dataset named path lifecycle and Pyramid hierarchy parameter validation.

Test Evidence

  • make fmt
  • make lint
  • make test
  • make cov, run tests, and collect coverage
  • Other (describe below)

Test details:

git diff --check

Local compile/test was not run because this local environment does not have the required VSAG build toolchain available. clang-format-15 and clang-tidy-15 are also not installed locally, so no alternate clang version was used. CI is expected to provide the compile/test validation.

Compatibility Impact

  • API/ABI compatibility: additive public Dataset APIs and Pyramid parameter constants.
  • Behavior changes: legacy Pyramid behavior remains unchanged unless reserved hierarchy parameters are explicitly supplied; those fail with a clear not-implemented error until later PRs implement execution.

Performance and Concurrency Impact

  • Performance impact: none expected; this PR only adds API/parameter parsing and Dataset metadata handling.
  • Concurrency/thread-safety impact: none expected.

Documentation Impact

  • No docs update needed
  • Updated docs:
    • README.md
    • DEVELOPMENT.md
    • CONTRIBUTING.md
    • Other:

Risk and Rollback

  • Risk level: low
  • Rollback plan: revert this PR; no serialized format or Pyramid storage changes are introduced.

Checklist

  • I have linked the relevant issue (required for kind/bug and kind/feature; see "Linked Issue" above)
  • I have added/updated tests for new behavior or bug fixes
  • I have considered API compatibility impact
  • I have updated docs if behavior/workflow changed
  • My commit messages follow project conventions (Conventional Commits, optional [skip ci] prefix)

Signed-off-by: JiangChao <jacllovey@qq.com>
Assisted-by: Codex:gpt-5
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 26, 2026

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Require kind label

Wonderful, this rule succeeded.
  • label~=^kind/

🟢 Require version label

Wonderful, this rule succeeded.
  • label~=^version/

🟢 Require linked issue for feature/bug PRs

Wonderful, this rule succeeded.
  • body~=(?im)(?:^|[\s\-\*])(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s+(?:#\d+|[\w.\-]+/[\w.\-]+#\d+|https?://github\.com/[\w.\-]+/[\w.\-]+/issues/\d+)

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for named hierarchies within the Dataset class, allowing paths to be associated with specific hierarchies. It also adds parsing, serialization, and compatibility checks for pyramid hierarchy parameters. Currently, named hierarchy operations (search, add, build) are reserved but not implemented in the Pyramid algorithm, throwing validation errors if used. The review feedback points out a robustness issue in PyramidHierarchyParameters::FromJson where a non-string, non-object JSON input could cause an unhandled exception, and suggests adding an explicit object type check.

Comment thread src/algorithm/pyramid_zparameters.cpp
Signed-off-by: jac <jacllovey@qq.com>
@jac0626 jac0626 requested a review from xfmeng17 May 26, 2026 05:35
jac0626 added 2 commits May 26, 2026 13:42
Signed-off-by: JiangChao <jacllovey@qq.com>
Assisted-by: Codex:gpt-5
Signed-off-by: JiangChao <jacllovey@qq.com>
Assisted-by: Codex:gpt-5
@jac0626 jac0626 force-pushed the codex/pyramid-hierarchy-api branch from aa836d7 to bb0cc29 Compare May 26, 2026 08:03
@jac0626 jac0626 self-assigned this May 26, 2026
@jac0626
Copy link
Copy Markdown
Collaborator Author

jac0626 commented May 26, 2026

CI failure note: the current Test X86 Functests failure matches the known flaky Pyramid duplicate test tracked in #2083, and does not appear related to this PR's dataset/API parameter changes.

Failure signature from this run:

./log/[pyramid].log failed
Pyramid Duplicate Test
tests/test_pyramid.cpp:675
REQUIRE( std::abs(distance) <= 2e-6 )
with expansion:
0.159739852f <= 0.000002

This is the same assertion described in #2083. Other checks in this run passed, including format, ASan build, compatibility, unit tests, examples, and TSAN tests. I think rerunning the failed functest job is the right handling here unless it reproduces with a different failure signature.

Signed-off-by: JiangChao <jacllovey@qq.com>
Assisted-by: Codex:gpt-5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feat](multiPyramid): define pyramid named hierarchy api parameters

1 participant