[
https://issues.apache.org/jira/browse/TINKERPOP-2742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17539531#comment-17539531
]
ASF GitHub Bot commented on TINKERPOP-2742:
-------------------------------------------
li-boxuan commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r877020771
##########
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) {
Review Comment:
I totally agree with your concerns and it makes sense to be cautious about
breaking changes. I would like to formulate the problems in two aspects:
1. Is it worth adding this new interface in general (with default behavior
preserved)?
Maybe. It might be impossible or discouraged to change the default
cardinality on the fly for some graph implementers. That being said, your
concerns about SubgraphStep, PartitionStrategy, etc. still holds here.
2. Is it worth introducing this breaking change in TinkerGraph?
Probably not due to the reasons you gave. However, I think it is still worth
considering the following actions:
a) Add a reminder to IO step on doc. I personally didn’t know that TinkerPop
would not infer cardinality from input file. The fact that you might lose data
(list/set properties) when you export and import again really confused me for
some time. Not sure if it is documented somewhere else, but I read the source
code to finally figure it out.
b) We can still infer the cardinality but we don’t really use it to preserve
old behavior. Instead, we give a warning to users when we detect discrepancy
between inferred cardinality and default cardinality.
> 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)
