fix(cpp): remove UB when reading unset builder vertex id

[Sober7135]

Reason for this PR

#846

What changes are included in this PR?

• Represent builder::Vertex id as std::optional<IdType> instead of an uninitialized scalar
• Make GetId() fail explicitly when the id is unset, instead of reading uninitialized memory
• Remove the stored empty_ flag from builder::Vertex
• Derive Empty() from properties_.empty() so vertex emptiness is no longer tracked by redundant mutable state
• Add/adjust tests to cover:
  • default-constructed vertex with unset id
  • SetId() / constructor-initialized id behavior
  • Empty() behavior after removing the stored empty_ field

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.87%. Comparing base (d535a43) to head (8255be0).

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #892      +/-   ##
============================================
+ Coverage     79.81%   79.87%   +0.05%     
  Complexity      615      615              
============================================
  Files            93       93              
  Lines         10296    10296              
  Branches       1055     1055              
============================================
+ Hits           8218     8224       +6     
+ Misses         1838     1832       -6     
  Partials        240      240              

Flag	Coverage Δ
cpp	70.91% <100.00%> (+0.11%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

[Copilot]

Pull request overview

This PR fixes undefined behavior in the C++ high-level builder API by ensuring builder::Vertex IDs are never read uninitialized, and updates C++/Python tests to reflect the new semantics (ID presence is independent from “emptiness”, which now refers to property payload).

Changes:

• Change builder::Vertex::id_ to std::optional<IdType> and add HasId(); make GetId() fail when unset.
• Remove redundant empty_ state from builder::Vertex and derive Empty() from properties_.empty().
• Add/adjust C++ and Python tests covering unset IDs, SetId(), and the updated Empty() behavior.

Reviewed changes

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

File	Description
cpp/src/graphar/high-level/vertices_builder.h	Switch vertex id storage to std::optional, add HasId(), and redefine Empty() based on property payload.
cpp/test/test_builder.cc	Add C++ coverage for unset-id behavior and updated Empty() semantics.
python/test/test_high_level_api.py	Add Python coverage for unset-id behavior and updated Empty() semantics.
python/src/bindings/high_level_binding.cc	Trivial formatting/line-number-only diff at file end.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

GetId() currently calls id_.value(), which throws std::bad_optional_access with an unhelpful default message when the id is unset (this also propagates as a generic runtime error in the Python bindings). Consider checking HasId() and throwing a more descriptive exception (or providing a non-throwing accessor) so users get a clear failure reason.

[Copilot]

The new doc comment says the id may be “assigned by VerticesBuilder”, but it doesn’t clarify whether that id is relative to the builder (0..N-1) or includes start_vertex_index_. Since AddVertex(..., index=-1) assigns vertices_.size() today, consider clarifying this wording to avoid misleading API users about what id they will observe.

[Copilot]

The test uses with pytest.raises(Exception) for GetId() on an unset id, which is very broad and can hide unrelated failures. It would be more robust to assert the specific exception type raised by the binding (and ideally match the message) so the test only passes for the intended behavior.

[yangxk1]

I agree this works! please note copilot's comment, thank you~

[Sober7135]

It seems that id_ inside builder::Vertex is never read and used when it is added into VerticesBuilder.

Because VerticesBuilder::AddVertex will always set the id_

incubator-graphar/cpp/src/graphar/high-level/vertices_builder.h

Lines 329 to 346 in 080a2f6

	Status AddVertex(
	Vertex& v, IdType index = -1, // NOLINT
	ValidateLevel validate_level = ValidateLevel::default_validate) {
	// validate
	GAR_RETURN_NOT_OK(validate(v, index, validate_level));
	// add a vertex
	if (index == -1) {
	v.SetId(vertices_.size());
	vertices_.push_back(v);
	} else {
	v.SetId(index);
	if (index >= static_cast<IdType>(vertices_.size()))
	vertices_.resize(index + 1);
	vertices_[index] = v;
	}
	num_vertices_++;
	return Status::OK();
	}

This is strange and misleading.
CC @yangxk1