TINKERPOP-2207 Provide SimpleValueMapStrategy

[dkuppitz]

https://issues.apache.org/jira/browse/TINKERPOP-2207

Implemented SimpleValueMapStrategy, which can be used by providers who do not support multi-valued properties.

gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
gremlin> g.V().valueMap()
==>[name:[marko],age:[29]]
==>[name:[vadas],age:[27]]
==>[name:[lop],lang:[java]]
==>[name:[josh],age:[32]]
==>[name:[ripple],lang:[java]]
==>[name:[peter],age:[35]]
gremlin> g.withStrategies(SimpleValueMapStrategy.instance()).V().valueMap()
==>[name:marko,age:29]
==>[name:vadas,age:27]
==>[name:lop,lang:java]
==>[name:josh,age:32]
==>[name:ripple,lang:java]
==>[name:peter,age:35]

VOTE +1

[spmallette]

This is an interesting way to deal with this problem. I say "problem" because as helpful as by(unfold()) is, it's annoying because you almost always have to type it. Making it so that you don't have to fuss with that extra step is nice.

at the same time though, it feels worrisome that we're saying that g.V().valueMap() can return a list of values in one graph but run that same query in another and you get singles. any thoughts on that concern?

[dkuppitz]

can return a list of values in one graph but run that same query in another and you get singles

That's why it's not a default strategy. To pass the test suite, all implementations still have to return lists (by default). So, I guess providers who do not support multi-valued properties, could do something like this when they initialize a traversal source:

if (!TESTING) {
  // add SimpleValueMapStrategy as default
}

[spmallette]

i understand it's not a default strategy but it will change the behavior of valueMap() per provider that decides to install it. seems like that will be confusing to users for them to do g.V().valueMap() with TinkerGraph to get lists but then go to another graph and get singles in a way that may not be clear to their understanding....it's not like they will immediately know the reason for the difference.

[dkuppitz]

So, what do you suggest? Promote this strategy to users instead? I thought about this, but I guess most users will then still be unaware of it (because they never read the docs) and keep complaining about the multi-valued valueMap values.

[spmallette]

I'm not sure I know what to suggest or that I was completely against the idea of solving it with a strategy that providers install. Maybe valueMap() should have worked as single from the beginning? at the time when valueMap() was created we didn't have by() all too well figured out or that it would be applied to valueMap() as we do now with valueMap().by(unfold()). if the default for valueMap() was single then we could reverse things to be valueMap().by(fold())`.

that seems like a better approach in the long run because:

1. I think most folks have graphs with single so this change would be better for the majority
2. if the user has list, valueMap() won't blow up during serialization because it will just default to the first item in the list - user is forced to explicitly by(fold()) (or something hopefully nicer) to force realization of the whole list of values. Then it's more the user's problem if they do and not ours.

I don't know how we would introduce this change from a versioning perspective. I just think that if we want to fix it, maybe we should just go the most direct route to doing it.

[dkuppitz]

Yeah, versioning is the only problem with that as it's a major breaking change.

Maybe it's not too bad to schedule it for 3.5. In the meantime, providers could implement a provider-specific strategy, which would just do what we have here in the PR. Once 3.5 comes out, they can remove their strategy (and it's no big problem if they forget to do it).

[spmallette]

I'm going to propose that we open 3.5.0 pretty soon - perhaps during code freeze week as we can then branch off tp34. I think we would do like we did with group() when we changed it's API and create a deprecated valueMapV3D4() with the old/current logic.

[spmallette]

Basically this comment summarizes reasoning and direction with this one at this point. Going to re-write the JIRA issue with that in mind.