[
https://issues.apache.org/jira/browse/TINKERPOP-2742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17538242#comment-17538242
]
ASF GitHub Bot commented on TINKERPOP-2742:
-------------------------------------------
mikepersonick commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r874888967
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java:
##########
@@ -291,12 +290,18 @@ public static Vertex createVertex(final
Attachable<Vertex> attachableVertex, fin
final Vertex vertex =
hostGraph.features().vertex().willAllowId(baseVertex.id()) ?
hostGraph.addVertex(T.id, baseVertex.id(), T.label,
baseVertex.label()) :
hostGraph.addVertex(T.label, baseVertex.label());
- baseVertex.properties().forEachRemaining(vp -> {
- final VertexProperty vertexProperty =
hostGraph.features().vertex().properties().willAllowId(vp.id()) ?
-
vertex.property(hostGraph.features().vertex().getCardinality(vp.key()),
vp.key(), vp.value(), T.id, vp.id()) :
-
vertex.property(hostGraph.features().vertex().getCardinality(vp.key()),
vp.key(), vp.value());
- vp.properties().forEachRemaining(p ->
vertexProperty.property(p.key(), p.value()));
- });
+ Map<String, List<VertexProperty<Object>>> propertyMap = new
HashMap<>();
Review Comment:
nit: final
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java:
##########
@@ -107,7 +107,7 @@ protected static boolean requiresPersistence(final Class<?>
test, final String t
*/
protected static boolean requiresListCardinalityAsDefault(final
LoadGraphWith.GraphData loadGraphWith,
final Class<?>
test, final String testMethodName) {
- return loadGraphWith == LoadGraphWith.GraphData.CREW
+ return loadGraphWith == LoadGraphWith.GraphData.CREW &&
!testMethodName.startsWith("shouldReadWriteCrew")
Review Comment:
Is it possible to fix the test case instead? I don't think we should hard
code test names into the graph provider code. If it's not possible to fix we'll
need to create an annotation for this case.
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -411,6 +404,15 @@ public boolean willAllowId(final Object id) {
public VertexProperty.Cardinality getCardinality(final String key) {
return defaultVertexPropertyCardinality;
}
+
+ @Override
+ public VertexProperty.Cardinality getCardinality(final String key,
final Object... values) {
+ VertexProperty.Cardinality cardinality =
defaultVertexPropertyCardinality;
Review Comment:
nit: final
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java:
##########
@@ -291,12 +290,18 @@ public static Vertex createVertex(final
Attachable<Vertex> attachableVertex, fin
final Vertex vertex =
hostGraph.features().vertex().willAllowId(baseVertex.id()) ?
hostGraph.addVertex(T.id, baseVertex.id(), T.label,
baseVertex.label()) :
hostGraph.addVertex(T.label, baseVertex.label());
- baseVertex.properties().forEachRemaining(vp -> {
- final VertexProperty vertexProperty =
hostGraph.features().vertex().properties().willAllowId(vp.id()) ?
-
vertex.property(hostGraph.features().vertex().getCardinality(vp.key()),
vp.key(), vp.value(), T.id, vp.id()) :
-
vertex.property(hostGraph.features().vertex().getCardinality(vp.key()),
vp.key(), vp.value());
- vp.properties().forEachRemaining(p ->
vertexProperty.property(p.key(), p.value()));
- });
+ Map<String, List<VertexProperty<Object>>> propertyMap = new
HashMap<>();
+ baseVertex.properties().forEachRemaining(vp ->
propertyMap.computeIfAbsent(vp.key(), k -> new ArrayList<>()).add(vp));
+ for (Map.Entry<String, List<VertexProperty<Object>>> entry :
propertyMap.entrySet()) {
+ VertexProperty.Cardinality cardinality =
hostGraph.features().vertex().getCardinality(
Review Comment:
nit: final
> IO read may use wrong cardinality for property
> ----------------------------------------------
>
> Key: TINKERPOP-2742
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2742
> Project: TinkerPop
> Issue Type: Bug
> Components: io
> Reporter: Boxuan Li
> Priority: Major
>
> g.io(...).read() might lose list/set properties. See the example below:
> {noformat}
> gremlin> graph = TinkerGraph.open()
> ==>tinkergraph[vertices:0 edges:0]
> gremlin> g = graph.traversal()
> ==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
> gremlin> v1 = g.addV().property("feature0", "0.0").property("feature0",
> "1.1").next()
> ==>v[0]
> gremlin> g.V().valueMap()
> ==>[feature0:[0.0,1.1]]
> gremlin> g.io("graph.json").write().iterate()
> gremlin> g.V().drop()
> gremlin> g.io("graph.json").read().iterate()
> gremlin> g.V().valueMap()
> ==>[feature0:[1.1]]{noformat}
> By verifying "graph.json", I am sure the write() step works fine. The problem
> is with read() step.
> In
> [https://github.com/apache/tinkerpop/blob/5fdc7d3b5174f73475ca1a48920d5dec614ffc0e/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java#L294-L298,]
> it relies on the host graph to decide the cardinality for the given property
> key. In TinkerGraph, `features().vertex().getCardinality(anything)` always
> returns default cardinality SINGLE (unless otherwise configured), which means
> all vertex properties are created with SINGLE cardinality, even if the graph
> file itself contains multiple values for that property, as shown in the above
> case.
>
> I presume TinkerGraph is not the only one who suffers from this problem. For
> example, for JanusGraph, if the default automatic schema maker is enabled,
> and a property wasn't defined explicitly, then SINGLE cardinality is returned
> by default. In this case, the loaded graph will be "wrong" in the sense that
> it turns LIST/SET values into a SINGLE value.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
