Skip to content

fix(cpp): preserve vertex IDs for nonzero builder starts - #970

Open
1fanwang wants to merge 6 commits into
apache:mainfrom
1fanwang:fix/vertex-index-offset
Open

fix(cpp): preserve vertex IDs for nonzero builder starts#970
1fanwang wants to merge 6 commits into
apache:mainfrom
1fanwang:fix/vertex-index-offset

Conversation

@1fanwang

Copy link
Copy Markdown

Reason for this PR

Writing vertices with a builder that starts after vertex 0 currently corrupts their IDs and serialized rows. The first auto-assigned vertex gets ID 0 instead of the requested starting ID, while an explicit global ID can create empty rows and move the payload to another ID.

Fixes #954.

What changes are included in this PR?

Convert explicit global IDs to local vector offsets before storing vertices, then restore the global ID on the vertex. The builder now enforces the nonnegative, chunk-aligned start and lower-bound index invariants under every validation level.

Are these changes tested?

The existing Test_vertices_builder now covers auto-assigned and explicit IDs with a nonzero start, verifies the persisted _graphArVertexIndex values, and checks invalid starts and below-start IDs under the default validation level.

Regression evidence

The regression test from this PR was first compiled against current main at 0e2bb53a7d273f70dfbac7e4a915911ceb27748d:

$ cd graphar
$ export GAR_TEST_DATA="$PWD/testing"
$ cpp/build_debug/test/test_builder "Test_vertices_builder"
FAILED:
  REQUIRE( auto_indexed_vertex.GetId() == nonzero_start_index )
with expansion:
  0 == 100
test cases: 1 | 0 passed | 1 failed

The same command at PR head 6113123c4d9423ca75773d36e7bbcbceded6b5ac passes:

$ cpp/build_debug/test/test_builder "Test_vertices_builder"
All tests passed (1158 assertions in 1 test case)

Are there any user-facing changes?

Yes. Vertices written by a builder with a nonzero start now retain their requested global IDs without emitting phantom rows. Invalid start and vertex indices return an index error instead of producing corrupt output.

Checklist

  • I performed a self-review of the code.
  • I ran GraphAr's C++ formatting and cpplint checks.
  • I ran pre-commit on the changed files.
  • I added tests that fail before the fix and pass after it.

Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
@codecov-commenter

codecov-commenter commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.76%. Comparing base (923f595) to head (6113123).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #970      +/-   ##
============================================
+ Coverage     77.59%   81.76%   +4.17%     
  Complexity      682      682              
============================================
  Files            85       96      +11     
  Lines          9091    11289    +2198     
  Branches       1098     1098              
============================================
+ Hits           7054     9231    +2177     
- Misses         1780     1801      +21     
  Partials        257      257              
Flag Coverage Δ
cpp 71.98% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SYaoJun
SYaoJun requested a review from Sober7135 September 7, 2026 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(cpp): VerticesBuilder::AddVertex mixes gobal vertex IDs with local storage indices

2 participants