[
https://issues.apache.org/jira/browse/TINKERPOP-2742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17539503#comment-17539503
]
ASF GitHub Bot commented on TINKERPOP-2742:
-------------------------------------------
spmallette commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r876979160
##########
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:
This changes the behavior of TinkerGraph in relation to attachable because
it overrides the default cardinality based on the data in the attachable rather
than letting the TinkerGraph configuration for cardinality do its job. I'm not
sure that's wrong, but it is different and a breaking change. I honestly don't
know which approach is more intuitive for a user:
1. Currently: The graph has a global configuration for cardinality. If you
were reading in some data from a GraphSON file you would have to know what that
it contained multi-properties and then configure the graph to use `list` or
`set` rather than the default of `single` to get it to read properly.
2. After this change: The graph still has a global configuration for
cardinality, except when reading in some data (i.e. using attachables) at which
point the user gets a dynamic behavior to individually decide per
property/value what the cardinality will be.
The fact that it is an exception and relevant only to attachables is what
gives me pause. Also, what about `SubgraphStep`, `PartitionStrategy` and other
places where the current `getCardinality(String)` is used? Should this behavior
be there as well somehow?
Ultimately, I think there are two questions:
1. Is it helpful for TinkerGraph's behavior to change such that the default
cardinality configuration can be overridden dynamically (perhaps that too is a
configuration to preserve the old behavior)?
2. If it is helpful and we want that change, how is it used universally with
as few exceptions as possible and if we can't get rid of all exceptions can we
live with those that must stay?
@krlawrence any thoughts on this?
> 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)
