acezen commented on code in PR #518:
URL: https://github.com/apache/incubator-graphar/pull/518#discussion_r1635866728
##########
maven-projects/target/.plxarc:
##########
@@ -0,0 +1 @@
+maven-shared-archive-resources
Review Comment:
The `target` directory is generated and don't include to the PR.
##########
cpp/test/test_arrow_chunk_reader.cc:
##########
@@ -34,8 +34,7 @@ namespace graphar {
TEST_CASE_METHOD(GlobalFixture, "ArrowChunkReader") {
// read file and construct graph info
- std::string path =
- test_data_dir + "/ldbc_sample/parquet/ldbc_sample.graph.yml";
+ std::string path = test_data_dir + "/ldbc_sample/LdbcSample.graph.yml";
Review Comment:
You can add a test case to use the json test data, it's better not modify
the current test case. like:
TEST_CASE_METHOD(GlobalFixture, "TestJson") {
}
##########
cpp/test/test_arrow_chunk_reader.cc:
##########
@@ -95,8 +94,9 @@ TEST_CASE_METHOD(GlobalFixture, "ArrowChunkReader") {
}
SECTION("CastDataType") {
- std::string prefix = test_data_dir + "/modern_graph/";
- std::string vertex_info_path = prefix + "person.vertex.yml";
+ /*std::string prefix = test_data_dir + "/modern_graph/";*/
Review Comment:
Ditto
##########
maven-projects/spark/datasources-32/src/main/scala/org/apache/spark/sql/graphar/GarTable.scala:
##########
@@ -31,8 +31,12 @@ import
org.apache.spark.sql.execution.datasources.v2.FileTable
import org.apache.spark.sql.graphar.csv.CSVWriteBuilder
import org.apache.spark.sql.graphar.orc.OrcWriteBuilder
import org.apache.spark.sql.graphar.parquet.ParquetWriteBuilder
+import org.apache.spark.sql.graphar.json.JSONWriteBuilder
import org.apache.spark.sql.types._
import org.apache.spark.sql.util.CaseInsensitiveStringMap
+import org.apache.spark.sql.execution.datasources.json.JsonDataSource
+//import org.apache.spark.sql.execution.datasources.v2.json.JsonWrite
Review Comment:
Is the JsonWrite no need to import? If so, just remove it.
##########
cpp/src/filesystem.cc:
##########
@@ -31,7 +31,9 @@
#include "graphar/util/expression.h"
#include "graphar/util/filesystem.h"
-namespace graphar::detail {
+namespace graphar {
+namespace ds = arrow::dataset;
+namespace detail {
Review Comment:
Is there any reason to change that? We have just updated to the nested
namespace in #489
##########
maven-projects/target/maven-shared-archive-resources/META-INF/NOTICE:
##########
@@ -0,0 +1,6 @@
+Apache GraphAr Root POM
Review Comment:
Ditto
##########
maven-projects/target/maven-shared-archive-resources/META-INF/LICENSE:
##########
@@ -0,0 +1,202 @@
+
Review Comment:
Ditto
##########
maven-projects/target/maven-shared-archive-resources/META-INF/DEPENDENCIES:
##########
@@ -0,0 +1,11 @@
+// ------------------------------------------------------------------
Review Comment:
Ditto
--
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]
