Copilot commented on code in PR #820:
URL: https://github.com/apache/incubator-graphar/pull/820#discussion_r2659281691
##########
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/PropertyGroupYaml.java:
##########
@@ -46,10 +46,19 @@ public PropertyGroupYaml(PropertyGroup propertyGroup) {
}
public PropertyGroup toPropertyGroup() {
+ String pgPrefix = prefix;
+ if (pgPrefix == null || pgPrefix.isEmpty()) {
+ StringBuilder prefixBuilder = new StringBuilder();
+ for (PropertyYaml property : properties) {
+ prefixBuilder.append(property.getName()).append("_");
+ }
+ prefixBuilder.append("/");
+ pgPrefix = prefixBuilder.toString();
Review Comment:
Consider using String.join() or a stream-based approach for better
readability and efficiency. For example: `String.join("_",
properties.stream().map(PropertyYaml::getName).collect(Collectors.toList())) +
"/"` would be cleaner than manually appending underscores in a loop. Note: this
suggestion assumes the trailing underscore issue is fixed first.
```suggestion
String joinedNames =
properties.stream()
.map(PropertyYaml::getName)
.collect(Collectors.joining("_"));
pgPrefix = joinedNames + "/";
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/PropertyTest.java:
##########
@@ -171,4 +175,19 @@ public void testPropertyImmutability() {
TestVerificationUtils.verifyProperty(prop, "test", true, false);
Assert.assertEquals(DataType.INT32, prop.getDataType());
}
+
+ @Test
+ public void testLoadPropertyDateTypes() throws IOException {
+ String testDataRoot = TestUtil.getTestData();
+ String timestampEdge =
+ testDataRoot +
"/ldbc_sample/parquet/person_knows-timestamp_person.edge.yml";
+ LocalFileSystemStringGraphInfoLoader
localFileSystemStringGraphInfoLoader =
+ new LocalFileSystemStringGraphInfoLoader();
+ EdgeInfo edgeInfo =
+
localFileSystemStringGraphInfoLoader.loadEdgeInfo(URI.create(timestampEdge));
+ Assert.assertTrue(edgeInfo.dump().contains("data_type: timestamp"));
+ String dateEdge = testDataRoot +
"/ldbc_sample/parquet/person_knows-date_person.edge.yml";
+ edgeInfo =
localFileSystemStringGraphInfoLoader.loadEdgeInfo(URI.create(dateEdge));
Review Comment:
The variable name "localFileSystemStringGraphInfoLoader" is unnecessarily
long. Following the pattern used in other test files in this codebase (see
GraphInfoLoaderTest.java, GraphInfoSaverTest.java, MultiPropertyTest.java),
consider using a shorter name like "loader" or "graphInfoLoader" for better
readability.
```suggestion
LocalFileSystemStringGraphInfoLoader loader =
new LocalFileSystemStringGraphInfoLoader();
EdgeInfo edgeInfo =
loader.loadEdgeInfo(URI.create(timestampEdge));
Assert.assertTrue(edgeInfo.dump().contains("data_type: timestamp"));
String dateEdge = testDataRoot +
"/ldbc_sample/parquet/person_knows-date_person.edge.yml";
edgeInfo = loader.loadEdgeInfo(URI.create(dateEdge));
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/PropertyGroupYaml.java:
##########
@@ -46,10 +46,19 @@ public PropertyGroupYaml(PropertyGroup propertyGroup) {
}
public PropertyGroup toPropertyGroup() {
+ String pgPrefix = prefix;
+ if (pgPrefix == null || pgPrefix.isEmpty()) {
+ StringBuilder prefixBuilder = new StringBuilder();
+ for (PropertyYaml property : properties) {
+ prefixBuilder.append(property.getName()).append("_");
+ }
+ prefixBuilder.append("/");
+ pgPrefix = prefixBuilder.toString();
Review Comment:
The default prefix generation logic does not handle the case when the
properties list is empty. If properties is empty, the loop will not execute and
the prefix will be just "/", which may not be a meaningful default. Consider
adding a check for empty properties and either throw an exception or use a
different default value like "property_group/".
```suggestion
if (properties == null || properties.isEmpty()) {
// Fallback default prefix when no properties are available
pgPrefix = "property_group/";
} else {
StringBuilder prefixBuilder = new StringBuilder();
for (PropertyYaml property : properties) {
prefixBuilder.append(property.getName()).append("_");
}
prefixBuilder.append("/");
pgPrefix = prefixBuilder.toString();
}
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/PropertyTest.java:
##########
@@ -35,6 +38,7 @@ public void setUp() {
idProperty = TestDataFactory.createIdProperty();
nameProperty = TestDataFactory.createProperty("name", DataType.STRING,
false, true);
optionalProperty = TestDataFactory.createProperty("optional",
DataType.STRING, false, true);
+ TestUtil.checkTestData();
Review Comment:
TestUtil.checkTestData() should be called once per test class rather than
before each test method. Consider moving this to a @BeforeClass method (like
GraphInfoLoaderTest does) to improve test performance, as checking test data
availability multiple times per test class is unnecessary.
##########
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/PropertyGroupYaml.java:
##########
@@ -46,10 +46,19 @@ public PropertyGroupYaml(PropertyGroup propertyGroup) {
}
public PropertyGroup toPropertyGroup() {
+ String pgPrefix = prefix;
+ if (pgPrefix == null || pgPrefix.isEmpty()) {
+ StringBuilder prefixBuilder = new StringBuilder();
+ for (PropertyYaml property : properties) {
+ prefixBuilder.append(property.getName()).append("_");
Review Comment:
The default prefix generation has a trailing underscore before the slash
(e.g., "property1_property2_/"). Based on the C++ reference implementation
mentioned in issue #814, the prefix should concatenate property names with
underscores but should not have a trailing underscore before the final slash.
The last underscore should be removed to generate a prefix like
"property1_property2/" instead of "property1_property2_/".
```suggestion
for (int i = 0; i < properties.size(); i++) {
if (i > 0) {
prefixBuilder.append("_");
}
prefixBuilder.append(properties.get(i).getName());
```
--
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]
