Copilot commented on code in PR #781:
URL: https://github.com/apache/incubator-graphar/pull/781#discussion_r2558462554
##########
maven-projects/info/src/main/java/org/apache/graphar/info/saver/BaseGraphInfoSaver.java:
##########
@@ -48,13 +53,22 @@ public void save(URI graphInfoUri, GraphInfo graphInfo)
throws IOException {
@Override
public void save(URI vertexInfoUri, VertexInfo vertexInfo) throws
IOException {
+ // if vertexInfoUri is a directory then save to
${vertexInfo.type}.vertex.yml
Review Comment:
The comment mentions ".vertex.yml" but the code creates ".vertex.yaml" (line
59). Update the comment to use ".vertex.yaml" for consistency.
```suggestion
// if vertexInfoUri is a directory then save to
${vertexInfo.type}.vertex.yaml
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/saver/BaseGraphInfoSaver.java:
##########
@@ -32,6 +32,11 @@ public abstract class BaseGraphInfoSaver implements
GraphInfoSaver {
@Override
public void save(URI graphInfoUri, GraphInfo graphInfo) throws IOException
{
+ // if graphInfoUri is a directory then save to
${graphInfo.name}.graph.yml
Review Comment:
The comment mentions ".graph.yml" but the code creates ".graph.yaml" (line
38). Update the comment to use ".graph.yaml" for consistency.
```suggestion
// if graphInfoUri is a directory then save to
${graphInfo.name}.graph.yaml
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/PropertyYaml.java:
##########
@@ -28,12 +28,14 @@ public class PropertyYaml {
private String data_type;
private boolean is_primary;
private Optional<Boolean> is_nullable;
+ private String cardinality;
public PropertyYaml() {
this.name = "";
this.data_type = "";
this.is_primary = false;
this.is_nullable = Optional.empty();
+ this.cardinality = "single"; // Default to single
Review Comment:
The cardinality field is added to PropertyYaml but is never used when
constructing a Property object (see line 48-54 toProperty() method). This means
cardinality values from YAML will be ignored during deserialization. Either
remove this field if it's not needed, or update the Property class and
toProperty() method to handle cardinality.
##########
maven-projects/info/src/test/java/org/apache/graphar/info/VertexInfoLabelsTest.java:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.graphar.info;
+
+import java.io.IOException;
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.graphar.info.loader.GraphInfoLoader;
+import
org.apache.graphar.info.loader.impl.LocalFileSystemStringGraphInfoLoader;
+import org.apache.graphar.info.saver.GraphInfoSaver;
+import org.apache.graphar.info.saver.impl.LocalFileSystemYamlGraphSaver;
+import org.apache.graphar.info.type.DataType;
+import org.apache.graphar.info.type.FileType;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class VertexInfoLabelsTest extends BaseFileSystemTest {
+
+ private String testSaveDirectory;
+ private GraphInfoSaver graphInfoSaver;
+ private GraphInfoLoader graphInfoLoader;
+
+ @Before
+ public void setUp() {
+ testSaveDirectory =
createCleanTestDirectory("ldbc_multi_labels_sample/");
+ graphInfoSaver = new LocalFileSystemYamlGraphSaver();
+ graphInfoLoader = new LocalFileSystemStringGraphInfoLoader();
+ }
+
+ @After
+ public void tearDown() {
+ // Test data will be preserved for debugging - cleanup happens before
next test run
+ System.out.println("Test data saved in: " + testSaveDirectory);
+ }
+
+ @Test
+ public void testVertexInfoWithLabels() throws IOException {
+ // Create property group
+ Property property = new Property("id", DataType.INT32, true, false);
+ PropertyGroup propertyGroup =
+ new PropertyGroup(Collections.singletonList(property),
FileType.PARQUET, "test/");
+
+ // Create labels
+ List<String> labels = Arrays.asList("Person", "Employee", "User");
+
+ // Create vertex info with labels
+ VertexInfo vertexInfo =
+ new VertexInfo(
+ "person",
+ 100L,
+ Collections.singletonList(propertyGroup),
+ labels,
+ "vertex/person/",
+ "gar/v1");
+
+ // Test getters
+ Assert.assertEquals("person", vertexInfo.getType());
+ Assert.assertEquals(100L, vertexInfo.getChunkSize());
+ Assert.assertEquals(labels, vertexInfo.getLabels());
+ Assert.assertEquals("vertex/person/", vertexInfo.getPrefix());
+ Assert.assertEquals("gar/v1", vertexInfo.getVersion().toString());
+
+ // Test property group related methods
+ Assert.assertEquals(1, vertexInfo.getPropertyGroupNum());
+ Assert.assertTrue(vertexInfo.hasProperty("id"));
+ Assert.assertTrue(vertexInfo.isPrimaryKey("id"));
+ Assert.assertFalse(vertexInfo.isNullableKey("id"));
+
+ // Test validation
+ Assert.assertTrue(vertexInfo.isValidated());
+
+ // Test dump
+ graphInfoSaver.save(URI.create(testSaveDirectory), vertexInfo);
+ // test load
+ VertexInfo loadedVertexInfo =
+ graphInfoLoader.loadVertexInfo(
+ URI.create(testSaveDirectory + "person.vertex.yaml"));
+ Assert.assertTrue(TestVerificationUtils.equalsVertexInfo(vertexInfo,
loadedVertexInfo));
+ }
+
+ @Test
+ public void testVertexInfoWithoutLabels() throws IOException {
+ // Create property group
+ Property property = new Property("id", DataType.INT32, true, false);
+ PropertyGroup propertyGroup =
+ new PropertyGroup(Collections.singletonList(property),
FileType.PARQUET, "test/");
+
+ // Create vertex info without labels (using old constructor)
+ VertexInfo vertexInfo =
+ new VertexInfo(
+ "person",
+ 100L,
+ Collections.singletonList(propertyGroup),
+ "vertex/person/",
+ "gar/v1");
+
+ // Test that labels list is empty but not null
+ Assert.assertEquals("person", vertexInfo.getType());
+ Assert.assertEquals(100L, vertexInfo.getChunkSize());
+ Assert.assertNotNull(vertexInfo.getLabels());
+ Assert.assertTrue(vertexInfo.getLabels().isEmpty());
+ Assert.assertEquals("vertex/person/", vertexInfo.getPrefix());
+
+ // Test validation
+ Assert.assertTrue(vertexInfo.isValidated());
+ // Test dump
+ graphInfoSaver.save(URI.create(testSaveDirectory), vertexInfo);
+ // test load
+ VertexInfo loadedVertexInfo =
+ graphInfoLoader.loadVertexInfo(
+ URI.create(testSaveDirectory + "person.vertex.yaml"));
+ Assert.assertTrue(TestVerificationUtils.equalsVertexInfo(vertexInfo,
loadedVertexInfo));
+ }
+
+ @Test
+ public void testVertexInfoWithEmptyLabels() throws IOException {
+ // Create property group
+ Property property = new Property("id", DataType.INT32, true, false);
+ PropertyGroup propertyGroup =
+ new PropertyGroup(Collections.singletonList(property),
FileType.PARQUET, "test/");
+
+ // Create vertex info with empty labels
+ VertexInfo vertexInfo =
+ new VertexInfo(
+ "person",
+ 100L,
+ Collections.singletonList(propertyGroup),
+ Collections.emptyList(),
+ "vertex/person/",
+ "gar/v1");
+
+ // Test that labels list is empty but not null
+ Assert.assertEquals("person", vertexInfo.getType());
+ Assert.assertEquals(100L, vertexInfo.getChunkSize());
+ Assert.assertNotNull(vertexInfo.getLabels());
+ Assert.assertTrue(vertexInfo.getLabels().isEmpty());
+ Assert.assertEquals("vertex/person/", vertexInfo.getPrefix());
+
+ // Test validation
+ Assert.assertTrue(vertexInfo.isValidated()); // Test dump
Review Comment:
The comment "// Test dump" appears on the same line as a validation
assertion. This comment should either be moved to line 162 where the dump
operation actually occurs, or the line should be formatted as two separate
lines for clarity.
```suggestion
Assert.assertTrue(vertexInfo.isValidated());
// Test dump
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/VertexYaml.java:
##########
@@ -80,6 +83,17 @@ public void setProperty_groups(List<PropertyGroupYaml>
property_groups) {
this.property_groups = property_groups;
}
+ public List<String> getLabels() {
+ if (labels == null) {
+ return null;
+ }
+ return labels.isEmpty() ? null : labels;
+ }
Review Comment:
The getLabels() method returns null for both null and empty lists (line 90).
This appears to be intentional for YAML serialization (to omit the labels field
when empty), but it's not documented. Consider adding a Javadoc comment
explaining this behavior: "Returns the labels list, or null if the list is null
or empty. Returning null for empty lists ensures that the labels field is
omitted from YAML output when no labels are defined."
##########
maven-projects/info/src/main/java/org/apache/graphar/info/saver/BaseGraphInfoSaver.java:
##########
@@ -48,13 +53,22 @@ public void save(URI graphInfoUri, GraphInfo graphInfo)
throws IOException {
@Override
public void save(URI vertexInfoUri, VertexInfo vertexInfo) throws
IOException {
+ // if vertexInfoUri is a directory then save to
${vertexInfo.type}.vertex.yml
+ if (vertexInfoUri.getPath().endsWith("/")) {
+ vertexInfoUri =
+ URI.create(vertexInfoUri.toString() + vertexInfo.getType()
+ ".vertex.yaml");
+ }
Writer vertexWriter = writeYaml(vertexInfoUri);
vertexInfo.dump(vertexWriter);
vertexWriter.close();
}
@Override
public void save(URI edgeInfoUri, EdgeInfo edgeInfo) throws IOException {
+ // if edgeInfoUri is a directory then save to
${edgeInfo.getConcat()}.edge.yml
Review Comment:
The comment mentions ".edge.yml" but the code creates ".edge.yaml" (line
70). Update the comment to use ".edge.yaml" for consistency.
```suggestion
// if edgeInfoUri is a directory then save to
${edgeInfo.getConcat()}.edge.yaml
```
--
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]
