Copilot commented on code in PR #916:
URL: https://github.com/apache/incubator-graphar/pull/916#discussion_r3201468253
##########
cpp/test/test_graph.cc:
##########
@@ -21,10 +21,16 @@
#include "./util.h"
#include "graphar/api/high_level_reader.h"
+#include "graphar/expression.h"
+#include "graphar/label.h"
+#include "graphar/reader_util.h"
Review Comment:
`graphar/label.h` and `graphar/reader_util.h` are included but not used in
this test file. Please remove these unused includes to keep dependencies
minimal and avoid unnecessary rebuilds.
##########
cpp/test/test_graph.cc:
##########
@@ -113,6 +119,47 @@ TEST_CASE_METHOD(GlobalFixture, "Graph") {
}
}
+ SECTION("VerticesCollectionFilterByProperty") {
+ std::string path =
+ test_data_dir + "/ldbc_sample/parquet/ldbc_sample.graph.yml";
+ auto maybe_graph_info = GraphInfo::Load(path);
+ REQUIRE(maybe_graph_info.status().ok());
+ auto graph_info = maybe_graph_info.value();
+
Review Comment:
This section reloads the same `ldbc_sample.graph.yml` that is already loaded
before the `SECTION`s in this test case. Because Catch2 re-runs the pre-SECTION
setup for each section, this results in `GraphInfo::Load()` being called twice
for this section; consider reusing the existing `graph_info` instead of
shadowing it here.
##########
cpp/test/test_graph.cc:
##########
@@ -113,6 +119,47 @@ TEST_CASE_METHOD(GlobalFixture, "Graph") {
}
}
+ SECTION("VerticesCollectionFilterByProperty") {
Review Comment:
The PR title/description mention adding a *label* filter test, but this new
section validates filtering by the `gender` property. Please either update the
PR metadata to match the change, or adjust the test/section name if the intent
was to cover label filtering.
##########
cpp/test/test_graph.cc:
##########
@@ -113,6 +119,47 @@ TEST_CASE_METHOD(GlobalFixture, "Graph") {
}
}
+ SECTION("VerticesCollectionFilterByProperty") {
+ std::string path =
+ test_data_dir + "/ldbc_sample/parquet/ldbc_sample.graph.yml";
+ auto maybe_graph_info = GraphInfo::Load(path);
+ REQUIRE(maybe_graph_info.status().ok());
+ auto graph_info = maybe_graph_info.value();
+
+ auto vertex_info = graph_info->GetVertexInfo("person");
+ REQUIRE(vertex_info != nullptr);
+
+ auto vertices = std::make_shared<VerticesCollection>(
+ vertex_info, graph_info->GetPrefix());
+ REQUIRE(vertices->size() == expectedTotalCount);
+ std::cout << "total size " << vertices->size() << std::endl;
+
+ auto filter_female =
+ _Equal(_Property("gender"), _Literal(std::string("female")));
+ std::vector<IdType> new_valid_chunk;
+ auto maybe_filtered_female_ids =
+ vertices->filter("gender", filter_female, &new_valid_chunk);
+ REQUIRE(maybe_filtered_female_ids.status().ok());
+ auto filtered_female_ids = maybe_filtered_female_ids.value();
+
+ auto filter_male =
+ _Equal(_Property("gender"), _Literal(std::string("male")));
+ auto maybe_filtered_male_ids =
+ vertices->filter("gender", filter_male, &new_valid_chunk);
Review Comment:
`new_valid_chunk` is reused across the female and male filter calls without
being cleared. If `VerticesCollection::filter()` appends to this vector (e.g.,
when filtering an already-filtered collection), this can make the output
ambiguous; prefer using separate vectors per call, clearing it between calls,
or passing `nullptr` if the chunk list isn’t asserted in this test.
--
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]
