feat(java,info): support multi-labels in yaml #781

yangxk1 · 2025-09-29T07:10:38Z

Reason for this PR

close #779

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

codecov-commenter · 2025-09-29T07:11:35Z

Codecov Report

❌ Patch coverage is 64.10256% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.40%. Comparing base (919818b) to head (59c6a80).

Files with missing lines	Patch %	Lines
.../main/java/org/apache/graphar/info/VertexInfo.java	52.63%	7 Missing and 2 partials ⚠️
.../apache/graphar/info/saver/BaseGraphInfoSaver.java	37.50%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@ ## main #781 +/- ## ============================================ - Coverage 76.75% 76.40% -0.35%  - Complexity 562 575 +13  ============================================ Files 83 83 Lines 8800 8751 -49 Branches 1032 1015 -17 ============================================ - Hits 6754 6686 -68  - Misses 1826 1843 +17  - Partials 220 222 +2

Flag	Coverage Δ
java-info	`82.40% <64.10%> (-0.33%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

yangxk1 · 2025-10-09T06:06:22Z

Hey @Thespica, please help me review.

…n-yaml

Copilot

Pull request overview

This pull request adds support for multi-labels in vertex YAML definitions for the Java GraphAr implementation. The feature allows vertices to have multiple labels (e.g., a vertex could be labeled as both "Person" and "Employee"), which is useful for graph data modeling with overlapping entity types.

Key Changes

Added labels field to VertexInfo class with support for multiple labels per vertex type
Updated YAML serialization/deserialization to handle the labels field in vertex definitions
Introduced comprehensive test coverage for vertices with labels, without labels, and empty labels

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
maven-projects/info/src/test/java/org/apache/graphar/info/VertexInfoLabelsTest.java	New test class with comprehensive coverage for multi-label vertex functionality including serialization/deserialization
maven-projects/info/src/main/java/org/apache/graphar/info/VertexInfo.java	Added labels field, updated constructors for backward compatibility, and extended builder pattern to support labels
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/VertexYaml.java	Added labels field and getter/setter methods for YAML serialization, returns null for empty lists to omit field
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/PropertyYaml.java	Added cardinality field to PropertyYaml for future property cardinality support in YAML
maven-projects/info/src/main/java/org/apache/graphar/info/loader/BaseGraphInfoLoader.java	Updated buildVertexInfoFromVertexYaml to pass labels when constructing VertexInfo
maven-projects/info/src/main/java/org/apache/graphar/info/saver/BaseGraphInfoSaver.java	Enhanced save methods to handle directory URIs by auto-generating filenames
maven-projects/info/src/test/java/org/apache/graphar/info/TestVerificationUtils.java	Added labels comparison to equalsVertexInfo for test verification
maven-projects/info/src/test/java/org/apache/graphar/info/TestUtil.java	Added helper methods to load LDBC graph with multi-label test data
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java	Added assertion to verify empty labels for existing test data without labels

Comments suppressed due to low confidence (2)

maven-projects/info/src/main/java/org/apache/graphar/info/yaml/PropertyYaml.java:46

The PropertyYaml(Property property) constructor doesn't set the cardinality field, so when a Property is serialized to YAML via this constructor, cardinality will remain as the default value "single" regardless of what might have been the intended cardinality. This creates inconsistent behavior where cardinality is only preserved when loading from YAML but not when saving to YAML from a Property object.

 public PropertyYaml(Property property) { this.name = property.getName(); this.data_type = property.getDataType().toString(); this.is_primary = property.isPrimary(); this.is_nullable = Optional.of(property.isNullable()); }

maven-projects/info/src/test/java/org/apache/graphar/info/TestUtil.java:77

The buildVertexInfoFromYaml method is missing the labels parameter when constructing VertexInfo. It should pass vertexYaml.getLabels() as the fourth parameter to match the updated VertexInfo constructor signature (see BaseGraphInfoLoader.buildVertexInfoFromVertexYaml line 79 for the correct implementation). This will cause labels to be lost when using this helper method.

 public static VertexInfo buildVertexInfoFromYaml(VertexYaml vertexYaml) { return new VertexInfo( vertexYaml.getType(), vertexYaml.getChunk_size(), vertexYaml.getProperty_groups().stream() .map(PropertyGroupYaml::toPropertyGroup) .collect(Collectors.toList()), vertexYaml.getPrefix(), vertexYaml.getVersion()); }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-25T03:53:09Z

maven-projects/info/src/main/java/org/apache/graphar/info/yaml/PropertyYaml.java

+ 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


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.

Copilot · 2025-11-25T03:53:10Z

maven-projects/info/src/main/java/org/apache/graphar/info/saver/BaseGraphInfoSaver.java


 @Override
 public void save(URI vertexInfoUri, VertexInfo vertexInfo) throws IOException {
+ // if vertexInfoUri is a directory then save to ${vertexInfo.type}.vertex.yml


The comment mentions ".vertex.yml" but the code creates ".vertex.yaml" (line 59). Update the comment to use ".vertex.yaml" for consistency.

Suggested change

// if vertexInfoUri is a directory then save to ${vertexInfo.type}.vertex.yml

// if vertexInfoUri is a directory then save to ${vertexInfo.type}.vertex.yaml

Copilot · 2025-11-25T03:53:10Z

maven-projects/info/src/main/java/org/apache/graphar/info/saver/BaseGraphInfoSaver.java


 @Override
 public void save(URI edgeInfoUri, EdgeInfo edgeInfo) throws IOException {
+ // if edgeInfoUri is a directory then save to ${edgeInfo.getConcat()}.edge.yml


The comment mentions ".edge.yml" but the code creates ".edge.yaml" (line 70). Update the comment to use ".edge.yaml" for consistency.

Suggested change

// if edgeInfoUri is a directory then save to ${edgeInfo.getConcat()}.edge.yml

// if edgeInfoUri is a directory then save to ${edgeInfo.getConcat()}.edge.yaml

Copilot · 2025-11-25T03:53:10Z

maven-projects/info/src/main/java/org/apache/graphar/info/saver/BaseGraphInfoSaver.java


 @Override
 public void save(URI graphInfoUri, GraphInfo graphInfo) throws IOException {
+ // if graphInfoUri is a directory then save to ${graphInfo.name}.graph.yml


The comment mentions ".graph.yml" but the code creates ".graph.yaml" (line 38). Update the comment to use ".graph.yaml" for consistency.

Suggested change

// if graphInfoUri is a directory then save to ${graphInfo.name}.graph.yml

// if graphInfoUri is a directory then save to ${graphInfo.name}.graph.yaml

Copilot · 2025-11-25T03:53:11Z

maven-projects/info/src/test/java/org/apache/graphar/info/VertexInfoLabelsTest.java

+ Assert.assertEquals("vertex/person/", vertexInfo.getPrefix());
+
+ // Test validation
+ Assert.assertTrue(vertexInfo.isValidated()); // Test dump


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.

Suggested change

Assert.assertTrue(vertexInfo.isValidated()); // Test dump

Assert.assertTrue(vertexInfo.isValidated());

// Test dump

Copilot · 2025-11-25T03:53:11Z

maven-projects/info/src/main/java/org/apache/graphar/info/yaml/VertexYaml.java

+ public List<String> getLabels() {
+ if (labels == null) {
+ return null;
+ }
+ return labels.isEmpty() ? null : labels;
+ }


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."

add multi labels

e4a41a9

yangxk1 requested a review from Thespica September 29, 2025 07:17

yangxk1 added 3 commits October 17, 2025 14:52

Merge remote-tracking branch 'apache' into 779-support-multi-labels-i…

22dc274

…n-yaml

fix

db726f7

format

59c6a80

yangxk1 requested a review from Copilot November 25, 2025 03:27

Copilot started reviewing on behalf of yangxk1 November 25, 2025 03:45 View session

Copilot finished reviewing on behalf of yangxk1 November 25, 2025 03:50

Copilot AI reviewed Nov 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(java,info): support multi-labels in yaml #781

feat(java,info): support multi-labels in yaml #781

Uh oh!

yangxk1 commented Sep 29, 2025

codecov-commenter commented Sep 29, 2025 •

edited

Loading

yangxk1 commented Oct 9, 2025

Copilot AI left a comment

Copilot AI Nov 25, 2025

Copilot AI Nov 25, 2025

Copilot AI Nov 25, 2025

Copilot AI Nov 25, 2025

Copilot AI Nov 25, 2025

Copilot AI Nov 25, 2025

Labels

2 participants

	// if vertexInfoUri is a directory then save to ${vertexInfo.type}.vertex.yml
	// if vertexInfoUri is a directory then save to ${vertexInfo.type}.vertex.yaml

	// if edgeInfoUri is a directory then save to ${edgeInfo.getConcat()}.edge.yml
	// if edgeInfoUri is a directory then save to ${edgeInfo.getConcat()}.edge.yaml

	// if graphInfoUri is a directory then save to ${graphInfo.name}.graph.yml
	// if graphInfoUri is a directory then save to ${graphInfo.name}.graph.yaml

	Assert.assertTrue(vertexInfo.isValidated()); // Test dump
	Assert.assertTrue(vertexInfo.isValidated());
	// Test dump

feat(java,info): support multi-labels in yaml #781

Are you sure you want to change the base?

feat(java,info): support multi-labels in yaml #781

Uh oh!

Conversation

yangxk1 commented Sep 29, 2025

Reason for this PR

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

codecov-commenter commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

yangxk1 commented Oct 9, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Labels

2 participants

codecov-commenter commented Sep 29, 2025 •

edited

Loading