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

1 participant