Copilot commented on code in PR #727:
URL: https://github.com/apache/incubator-graphar/pull/727#discussion_r2272275789
##########
maven-projects/info/src/main/java/org/apache/graphar/info/AdjacentList.java:
##########
@@ -19,42 +19,38 @@
package org.apache.graphar.info;
-import org.apache.graphar.proto.AdjListType;
-import org.apache.graphar.proto.FileType;
+import org.apache.graphar.info.type.AdjListType;
+import org.apache.graphar.info.type.FileType;
+import org.apache.graphar.info.yaml.AdjacentListYaml;
public class AdjacentList {
- private final org.apache.graphar.proto.AdjacentList protoAdjacentList;
+ private final AdjListType type;
+ private final FileType fileType;
+ private final String prefix;
public AdjacentList(AdjListType type, FileType fileType, String prefix) {
- protoAdjacentList =
- org.apache.graphar.proto.AdjacentList.newBuilder()
- .setType(type)
- .setFileType(fileType)
- .setPrefix(prefix)
- .build();
+ this.type = type;
+ this.fileType = fileType;
+ this.prefix = prefix;
}
- private AdjacentList(org.apache.graphar.proto.AdjacentList
protoAdjacentList) {
- this.protoAdjacentList = protoAdjacentList;
- }
-
- public static AdjacentList ofProto(org.apache.graphar.proto.AdjacentList
protoAdjacentList) {
- return new AdjacentList(protoAdjacentList);
+ AdjacentList(AdjacentListYaml yamlParser) {
+ this.type =
+ AdjListType.fromOrderedAndAlignedBy(
+ yamlParser.isOrdered(), yamlParser.isAligned_by());
Review Comment:
The method call should be `yamlParser.getAligned_by()` instead of
`yamlParser.isAligned_by()` since `aligned_by` is a String field, not a boolean.
```suggestion
yamlParser.isOrdered(), yamlParser.getAligned_by());
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/PropertyGroup.java:
##########
@@ -27,109 +27,80 @@
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;
-import org.apache.graphar.proto.DataType;
-import org.apache.graphar.proto.FileType;
+import org.apache.graphar.info.type.DataType;
+import org.apache.graphar.info.type.FileType;
+import org.apache.graphar.info.yaml.PropertyGroupYaml;
import org.apache.graphar.util.GeneralParams;
public class PropertyGroup implements Iterable<Property> {
- private final org.apache.graphar.proto.PropertyGroup protoPropertyGroup;
- private final List<Property> cachedPropertyList;
- private final Map<String, Property> cachedPropertyMap;
-
- public PropertyGroup(List<Property> propertyList, FileType fileType,
String prefix) {
- this.cachedPropertyList = List.copyOf(propertyList);
- this.cachedPropertyMap =
- cachedPropertyList.stream()
- .collect(
- Collectors.toUnmodifiableMap(
- Property::getName,
Function.identity()));
- this.protoPropertyGroup =
- org.apache.graphar.proto.PropertyGroup.newBuilder()
- .addAllProperties(
- cachedPropertyList.stream()
- .map(Property::getProto)
- .collect(Collectors.toList()))
- .setFileType(fileType)
- .setPrefix(prefix)
- .build();
- }
-
- private PropertyGroup(org.apache.graphar.proto.PropertyGroup
protoPropertyGroup) {
- this.protoPropertyGroup = protoPropertyGroup;
- this.cachedPropertyList =
- protoPropertyGroup.getPropertiesList().stream()
- .map(Property::ofProto)
- .collect(Collectors.toUnmodifiableList());
- this.cachedPropertyMap =
- cachedPropertyList.stream()
+ private final List<Property> propertyList;
+ private final Map<String, Property> propertyMap;
+ private final FileType fileType;
+ private final String prefix;
+
+ public PropertyGroup(List<Property> propertyMap, FileType fileType, String
prefix) {
+ this.propertyList = List.copyOf(propertyMap);
+ this.propertyMap =
+ propertyMap.stream()
.collect(
Collectors.toUnmodifiableMap(
Property::getName,
Function.identity()));
+ this.fileType = fileType;
+ this.prefix = prefix;
}
- public static PropertyGroup ofProto(org.apache.graphar.proto.PropertyGroup
protoPropertyGroup) {
- return new PropertyGroup(protoPropertyGroup);
+ PropertyGroup(PropertyGroupYaml yamlParser) {
+ this(
+ yamlParser.getProperties().stream()
+ .map(Property::new)
+ .collect(Collectors.toUnmodifiableList()),
+ FileType.fromString(yamlParser.getFile_type()),
+ yamlParser.getPrefix());
}
public Optional<PropertyGroup> addPropertyAsNew(Property property) {
- if (property == null ||
cachedPropertyMap.containsKey(property.getName())) {
+ if (property == null || !propertyMap.containsKey(property.getName())) {
Review Comment:
The logic is inverted - this should return empty when the property already
exists in the map, not when it doesn't exist. The condition should be
`propertyMap.containsKey(property.getName())` without the negation.
```suggestion
if (property == null || propertyMap.containsKey(property.getName()))
{
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/AdjacentList.java:
##########
@@ -19,42 +19,38 @@
package org.apache.graphar.info;
-import org.apache.graphar.proto.AdjListType;
-import org.apache.graphar.proto.FileType;
+import org.apache.graphar.info.type.AdjListType;
+import org.apache.graphar.info.type.FileType;
+import org.apache.graphar.info.yaml.AdjacentListYaml;
public class AdjacentList {
- private final org.apache.graphar.proto.AdjacentList protoAdjacentList;
+ private final AdjListType type;
+ private final FileType fileType;
+ private final String prefix;
public AdjacentList(AdjListType type, FileType fileType, String prefix) {
- protoAdjacentList =
- org.apache.graphar.proto.AdjacentList.newBuilder()
- .setType(type)
- .setFileType(fileType)
- .setPrefix(prefix)
- .build();
+ this.type = type;
+ this.fileType = fileType;
+ this.prefix = prefix;
}
- private AdjacentList(org.apache.graphar.proto.AdjacentList
protoAdjacentList) {
- this.protoAdjacentList = protoAdjacentList;
- }
-
- public static AdjacentList ofProto(org.apache.graphar.proto.AdjacentList
protoAdjacentList) {
- return new AdjacentList(protoAdjacentList);
+ AdjacentList(AdjacentListYaml yamlParser) {
+ this.type =
+ AdjListType.fromOrderedAndAlignedBy(
+ yamlParser.isOrdered(), yamlParser.isAligned_by());
+ this.fileType = FileType.valueOf(yamlParser.getFile_type());
Review Comment:
This should use `FileType.fromString(yamlParser.getFile_type())` instead of
`valueOf()` to be consistent with the custom fromString method that handles
lowercase strings.
```suggestion
this.fileType = FileType.fromString(yamlParser.getFile_type());
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/GraphInfo.java:
##########
@@ -94,121 +167,134 @@ public Optional<GraphInfo> addVertexAsNew(VertexInfo
vertexInfo) {
if (vertexInfo == null || hasVertexInfo(vertexInfo.getType())) {
return Optional.empty();
}
- final org.apache.graphar.proto.GraphInfo newProtoGraphInfo =
- org.apache.graphar.proto.GraphInfo.newBuilder(protoGraphInfo)
- .addVertices(vertexInfo.getVertexPath())
- .build();
- final List<VertexInfo> newVertexInfoList =
- Stream.concat(cachedVertexInfoList.stream(),
Stream.of(vertexInfo))
+ List<VertexInfo> newVertexInfos =
+ Stream.concat(vertexInfos.stream(), Stream.of(vertexInfo))
.collect(Collectors.toList());
- final Map<String, VertexInfo> newVertexInfoMap =
+ Map<String, VertexInfo> newVertexType2VertexInfo =
Stream.concat(
- cachedVertexInfoMap.entrySet().stream(),
+ vertexType2VertexInfo.entrySet().stream(),
Stream.of(Map.entry(vertexInfo.getType(),
vertexInfo)))
.collect(
Collectors.toUnmodifiableMap(
Map.Entry::getKey,
Map.Entry::getValue));
return Optional.of(
new GraphInfo(
- newProtoGraphInfo,
- newVertexInfoList,
- cachedEdgeInfoList,
- newVertexInfoMap,
- cachedEdgeInfoMap));
+ name,
+ newVertexInfos,
+ edgeInfos,
+ prefix,
+ version,
+ newVertexType2VertexInfo,
+ edgeConcat2EdgeInfo));
}
public Optional<GraphInfo> addEdgeAsNew(EdgeInfo edgeInfo) {
if (edgeInfo == null
|| hasEdgeInfo(
- edgeInfo.getSrcLabel(), edgeInfo.getEdgeLabel(),
edgeInfo.getDstLabel())) {
+ edgeInfo.getSrcType(), edgeInfo.getEdgeType(),
edgeInfo.getDstType())) {
return Optional.empty();
}
- final org.apache.graphar.proto.GraphInfo newProtoGraphInfo =
- org.apache.graphar.proto.GraphInfo.newBuilder(protoGraphInfo)
- .addEdges(edgeInfo.getEdgePath())
- .build();
- final List<EdgeInfo> newEdgeInfos =
- Stream.concat(cachedEdgeInfoList.stream(), Stream.of(edgeInfo))
- .collect(Collectors.toList());
- final Map<String, EdgeInfo> newEdgeConcat2EdgeInfo =
+ List<EdgeInfo> newEdgeInfos =
+ Stream.concat(edgeInfos.stream(),
Stream.of(edgeInfo)).collect(Collectors.toList());
+ Map<String, EdgeInfo> newEdgeConcat2EdgeInfo =
Stream.concat(
- cachedEdgeInfoMap.entrySet().stream(),
+ edgeConcat2EdgeInfo.entrySet().stream(),
Stream.of(Map.entry(edgeInfo.getConcat(),
edgeInfo)))
.collect(
Collectors.toUnmodifiableMap(
Map.Entry::getKey,
Map.Entry::getValue));
return Optional.of(
new GraphInfo(
- newProtoGraphInfo,
- cachedVertexInfoList,
+ name,
+ vertexInfos,
newEdgeInfos,
- cachedVertexInfoMap,
+ prefix,
+ version,
+ vertexType2VertexInfo,
newEdgeConcat2EdgeInfo));
}
- public boolean hasVertexInfo(String label) {
- return cachedVertexInfoMap.containsKey(label);
+ public boolean hasVertexInfo(String type) {
+ return vertexType2VertexInfo.containsKey(type);
}
- public boolean hasEdgeInfo(String srcLabel, String edgeLabel, String
dstLabel) {
- return cachedEdgeInfoMap.containsKey(EdgeInfo.concat(srcLabel,
edgeLabel, dstLabel));
+ public boolean hasEdgeInfo(String srcType, String edgeType, String
dstType) {
+ return edgeConcat2EdgeInfo.containsKey(srcType + dstType + edgeType);
}
- public VertexInfo getVertexInfo(String label) {
- checkVertexExist(label);
- return cachedVertexInfoMap.get(label);
+ public VertexInfo getVertexInfo(String type) {
+ checkVertexExist(type);
+ return vertexType2VertexInfo.get(type);
}
- public EdgeInfo getEdgeInfo(String srcLabel, String edgeLabel, String
dstLabel) {
- checkEdgeExist(srcLabel, edgeLabel, dstLabel);
- return cachedEdgeInfoMap.get(EdgeInfo.concat(srcLabel, edgeLabel,
dstLabel));
+ public EdgeInfo getEdgeInfo(String srcType, String edgeType, String
dstType) {
+ checkEdgeExist(srcType, edgeType, dstType);
+ return edgeConcat2EdgeInfo.get(srcType + edgeType + dstType);
}
Review Comment:
The concatenation order is inconsistent with the EdgeTriplet.getConcat()
method and should include separators. This should use the same pattern as
EdgeTriplet.getConcat().
```suggestion
return edgeConcat2EdgeInfo.get(getEdgeConcatKey(srcType, edgeType,
dstType));
}
/**
* Constructs the edge concat key using the same pattern as
EdgeTriplet.getConcat().
*/
private static String getEdgeConcatKey(String srcType, String edgeType,
String dstType) {
// Assuming EdgeTriplet.getConcat() uses srcType + separator +
edgeType + separator + dstType
return srcType + GeneralParams.regularSeparator + edgeType +
GeneralParams.regularSeparator + dstType;
}
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/GraphInfo.java:
##########
@@ -94,121 +167,134 @@ public Optional<GraphInfo> addVertexAsNew(VertexInfo
vertexInfo) {
if (vertexInfo == null || hasVertexInfo(vertexInfo.getType())) {
return Optional.empty();
}
- final org.apache.graphar.proto.GraphInfo newProtoGraphInfo =
- org.apache.graphar.proto.GraphInfo.newBuilder(protoGraphInfo)
- .addVertices(vertexInfo.getVertexPath())
- .build();
- final List<VertexInfo> newVertexInfoList =
- Stream.concat(cachedVertexInfoList.stream(),
Stream.of(vertexInfo))
+ List<VertexInfo> newVertexInfos =
+ Stream.concat(vertexInfos.stream(), Stream.of(vertexInfo))
.collect(Collectors.toList());
- final Map<String, VertexInfo> newVertexInfoMap =
+ Map<String, VertexInfo> newVertexType2VertexInfo =
Stream.concat(
- cachedVertexInfoMap.entrySet().stream(),
+ vertexType2VertexInfo.entrySet().stream(),
Stream.of(Map.entry(vertexInfo.getType(),
vertexInfo)))
.collect(
Collectors.toUnmodifiableMap(
Map.Entry::getKey,
Map.Entry::getValue));
return Optional.of(
new GraphInfo(
- newProtoGraphInfo,
- newVertexInfoList,
- cachedEdgeInfoList,
- newVertexInfoMap,
- cachedEdgeInfoMap));
+ name,
+ newVertexInfos,
+ edgeInfos,
+ prefix,
+ version,
+ newVertexType2VertexInfo,
+ edgeConcat2EdgeInfo));
}
public Optional<GraphInfo> addEdgeAsNew(EdgeInfo edgeInfo) {
if (edgeInfo == null
|| hasEdgeInfo(
- edgeInfo.getSrcLabel(), edgeInfo.getEdgeLabel(),
edgeInfo.getDstLabel())) {
+ edgeInfo.getSrcType(), edgeInfo.getEdgeType(),
edgeInfo.getDstType())) {
return Optional.empty();
}
- final org.apache.graphar.proto.GraphInfo newProtoGraphInfo =
- org.apache.graphar.proto.GraphInfo.newBuilder(protoGraphInfo)
- .addEdges(edgeInfo.getEdgePath())
- .build();
- final List<EdgeInfo> newEdgeInfos =
- Stream.concat(cachedEdgeInfoList.stream(), Stream.of(edgeInfo))
- .collect(Collectors.toList());
- final Map<String, EdgeInfo> newEdgeConcat2EdgeInfo =
+ List<EdgeInfo> newEdgeInfos =
+ Stream.concat(edgeInfos.stream(),
Stream.of(edgeInfo)).collect(Collectors.toList());
+ Map<String, EdgeInfo> newEdgeConcat2EdgeInfo =
Stream.concat(
- cachedEdgeInfoMap.entrySet().stream(),
+ edgeConcat2EdgeInfo.entrySet().stream(),
Stream.of(Map.entry(edgeInfo.getConcat(),
edgeInfo)))
.collect(
Collectors.toUnmodifiableMap(
Map.Entry::getKey,
Map.Entry::getValue));
return Optional.of(
new GraphInfo(
- newProtoGraphInfo,
- cachedVertexInfoList,
+ name,
+ vertexInfos,
newEdgeInfos,
- cachedVertexInfoMap,
+ prefix,
+ version,
+ vertexType2VertexInfo,
newEdgeConcat2EdgeInfo));
}
- public boolean hasVertexInfo(String label) {
- return cachedVertexInfoMap.containsKey(label);
+ public boolean hasVertexInfo(String type) {
+ return vertexType2VertexInfo.containsKey(type);
}
- public boolean hasEdgeInfo(String srcLabel, String edgeLabel, String
dstLabel) {
- return cachedEdgeInfoMap.containsKey(EdgeInfo.concat(srcLabel,
edgeLabel, dstLabel));
+ public boolean hasEdgeInfo(String srcType, String edgeType, String
dstType) {
+ return edgeConcat2EdgeInfo.containsKey(srcType + dstType + edgeType);
Review Comment:
The concatenation order is inconsistent with the EdgeTriplet.getConcat()
method which uses srcType + separator + edgeType + separator + dstType. This
should use EdgeInfo.concat() or the same pattern with separators.
```suggestion
return edgeConcat2EdgeInfo.containsKey(EdgeInfo.concat(srcType,
edgeType, dstType));
```
--
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]
