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?
--
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: commits-unsubscr...@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
