[
https://issues.apache.org/jira/browse/TINKERPOP-2742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17539156#comment-17539156
]
ASF GitHub Bot commented on TINKERPOP-2742:
-------------------------------------------
li-boxuan commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r876504970
##########
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:
The test `shouldReadWriteCrew` actually works fine without this change.
However, in the old logic, whenever the graph is `CREW`, we always use LIST
cardinality as the default cardinality in tests, which conceals the problem
described in
https://issues.apache.org/jira/projects/TINKERPOP/issues/TINKERPOP-2742.
While I agree hardcoding is not a good practice, this method already
hardcodes some test names including `shouldAttachWithCreateMethod` and
`testAttachableCreateMethod`. Since this is just a graph provider under the
`test` package, I presume it is okay to do so (although not perfect).
> 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)
