Copilot commented on code in PR #932:
URL: https://github.com/apache/incubator-graphar/pull/932#discussion_r3636296413
##########
cpp/test/test_builder.cc:
##########
@@ -280,6 +282,65 @@ TEST_CASE_METHOD(GlobalFixture, "test_edges_builder") {
REQUIRE(
builder->AddPropertyColumn("creationDate", string_values).IsInvalid());
+ // add property column via (src, dst) map
+ {
+ // build a new builder for map-based test
+ auto maybe_builder2 = builder::EdgesBuilder::Make(
+ edge_info, "/tmp/", AdjListType::ordered_by_dest, vertices_num);
+ REQUIRE(!maybe_builder2.has_error());
+ auto builder2 = maybe_builder2.value();
+
+ // add a few edges manually
+ REQUIRE(builder2->AddEdge(builder::Edge(0, 1)).ok());
+ REQUIRE(builder2->AddEdge(builder::Edge(0, 2)).ok());
+ REQUIRE(builder2->AddEdge(builder::Edge(1, 3)).ok());
+ REQUIRE(builder2->AddEdge(builder::Edge(2, 4)).ok());
+
+ // build map: (src, dst) -> value
+ std::unordered_map<std::pair<IdType, IdType>, std::any,
builder::PairIdHash>
+ value_map;
+ value_map[{0, 1}] = std::string("edge_0_1");
+ value_map[{0, 2}] = std::string("edge_0_2");
+ value_map[{1, 3}] = std::string("edge_1_3");
+ // deliberately omit (2, 4) to test null handling
+
+ REQUIRE(builder2->AddPropertyColumn("creationDate", value_map).ok());
+ REQUIRE(builder2->Dump().ok());
+
+ // verify: read back and check
+ auto parquet_file =
+ "/tmp/edge/person_knows_person/ordered_by_dest/creationDate/part0/"
+ "chunk0";
+ std::unique_ptr<parquet::arrow::FileReader> reader;
+ REQUIRE(graphar::util::OpenParquetArrowReader(
+ parquet_file, arrow::default_memory_pool(), &reader)
+ .ok());
+ auto maybe_table = reader->ReadTable();
+ REQUIRE(maybe_table.ok());
+ auto table = maybe_table.ValueOrDie();
+ auto col = table->GetColumnByName("creationDate");
+ REQUIRE(col != nullptr);
+ auto arr = std::static_pointer_cast<arrow::StringArray>(col->chunk(0));
+ REQUIRE(arr->length() == 4);
+
+ // Check that the mapped edges have the correct values
+ bool found_0_1 = false, found_0_2 = false, found_1_3 = false;
+ for (int i = 0; i < arr->length(); i++) {
+ if (arr->IsValid(i)) {
+ std::string val = arr->GetString(i);
+ if (val == "edge_0_1")
+ found_0_1 = true;
+ if (val == "edge_0_2")
+ found_0_2 = true;
+ if (val == "edge_1_3")
+ found_1_3 = true;
+ }
+ }
+ REQUIRE(found_0_1);
+ REQUIRE(found_0_2);
+ REQUIRE(found_1_3);
Review Comment:
The map-based test claims to exercise null handling for the omitted (2, 4)
edge, but it never asserts that a null was actually written, and it would still
pass even if an extra unexpected non-null value were present. Add explicit
assertions for null_count and the exact set/counts of expected strings.
##########
cpp/src/graphar/high-level/edges_builder.h:
##########
@@ -305,6 +320,31 @@ class EdgesBuilder {
}
return Status::OK();
}
+
+ /**
+ * @brief Add a property to edges in the collection by (src, dst) mapping.
+ *
+ * Edges whose (src_id, dst_id) is not present in the map will not have this
+ * property set (written as null later).
+ *
+ * @param property name of the property
+ * @param values map from (src_id, dst_id) to the property value
+ * @return Status: ok.
+ */
Review Comment:
This overload keys property values by (src_id, dst_id). If the builder
contains multiple edges with the same endpoints, the current implementation
will apply the same mapped value to all of them (and silently ignore any map
keys that don’t exist in the builder). The API/docs should make this behavior
explicit so callers don’t assume they can set distinct values for parallel
edges.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
