TINKERPOP-1483: valueMap should always return string keys #446

JPMoresmau · 2016-09-30T14:51:09Z

Code changed, test added

okram · 2016-09-30T14:59:00Z

Hm. This is a hard pill to swallow as its not backwards compatible. I was thinking you were going to go the route of making it Map<Object,Object> and thus, allow the enums as they are. If people have code that is map.get(id), your changes will break their code.

JPMoresmau · 2016-09-30T15:06:14Z

Well, I wasn't going to change the API on my very first PR :-). The first thing I did with a valueMap result was to iterate on the keys, which crashes... Of course if you want to change the API to Map<Object,Object> it's fine. But surely changing the api from Map<String,Object> to Map<Object,Object> is also going to break existing code? I suppose it will fail at compile time, and not at runtime, which is arguably better.

dkuppitz · 2016-10-28T21:51:39Z

I've seen lots of code where people actually use id as a property name.

gremlin> g = TinkerGraph.open().traversal()
==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
gremlin> g.addV().property("id", UUID.randomUUID())
==>v[0]
gremlin> g.V().valueMap(true)
==>[id:0,id:[02f29cc6-7f04-477c-a884-d5196064109a],label:vertex]
gremlin> g.V().project("vertexId","applicationId").by(id).by("id").next()
==>vertexId=0
==>applicationId=02f29cc6-7f04-477c-a884-d5196064109a

Those people would be in big trouble if we would merge this PR:

VOTE: -1

okram · 2016-10-29T21:13:48Z

Yea, this is a troublesome PR. VOTE -1.

JPMoresmau · 2016-10-30T11:42:03Z

Fine, but if you don't want to change the implementation to match the interface, you'll have to change the interface of valueMap... Having keys of a type that is not allowed by the actual generic signature of the map is not great...

dkuppitz · 2016-10-30T11:59:16Z

To support something like valueMap(T.label, "name")? This would be cool, but would go into another PR.

JPMoresmau · 2016-10-30T12:55:44Z

What? I'm just saying that currently valueMap returns Map<String,Object>, so if you iterate on all the keys as String, you get a runtime crash because there are keys that are not strings. So if you don't want to put only String as keys, you need to change the Map to Map<Object,Object> , which will also break existing code.

dkuppitz · 2016-10-30T15:18:14Z

Oh, now I see what you're saying. Yes, this would be the right way to go. The current PropertyMapStep implementation is pretty weird.

protected Map<String, E> map(...) {
    final Map<Object, Object> map = new HashMap<>();
    ...
    return (Map) map;
}

robertdale · 2016-11-23T15:15:03Z

Can this PR be updated with Map<Object, Object> or should it be closed and open a new one?

robertdale · 2016-11-25T10:43:49Z

This is interesting. I was unaware of some or didn't understand some implementation details. Taking a step back and looking at this again, I wonder if the original intent is more correct. Sorry @JPMoresmau
😃

We have to ask what is the purpose of Hidden? And what is the purpose of T.*.getAccessor()? Looks like Has step internally use T.*.getAccessor() for equality tests. Is this more of an internal method or should it be exposed?

Ultimately, the question is with what or how do we expect to access T tokens in a value map?

Given: map = g.V().valueMap(true).next()

Access by: map.get(T.id) or map.get(T.id.getAccessor())?

If the answer is T.id then the interface must be <Object,Object>

If the answer is T.id.getAccessor() then the interface must be <String,Object>

In either case, there is no conflict between system-level T tokens (e.g. T.id), as these are either Enum or Hidden, and user-level strings (e.g. id).

dkuppitz · 2016-11-25T23:58:59Z

Access by: map.get(T.id) or map.get(T.id.getAccessor())?

By map.get(T.id). Providers may allow property names that match TinkerPop's token accessors, hence map.get(T.id) must not be the same as map.get("id") / map.get(T.id.getAccessor()) and a user-defined id property may not overwrite and may not be overwritten by the internal element id.

robertdale · 2016-11-26T17:41:27Z

Thanks for the clarification @dkuppitz

./docker/build.sh -t -i -n passes

VOTE: +1

okram · 2016-11-28T17:02:21Z

Yea, Map<Object,Object> is the thing. :| ... I really don't like valueMap(). For this reason and for the boolean argument overload ... fuggly.

VOTE +1.

spmallette · 2016-11-28T17:28:11Z

This requires CHANGELOG and upgrade docs too (unless a committer wants to take responsibility for that).

okram · 2016-11-28T17:43:59Z

@dkuppitz -- will you handle merge and thus, do the CHANGELOG and update the upgrade docs on merge?

JPMoresmau · 2016-11-28T19:15:49Z

I've updated the changelog and upgrade doc, not sure it's sufficient, please check! thanks!

okram · 2016-11-28T19:36:08Z

CHANGELOG.asciidoc

@@ -46,6 +46,7 @@ TinkerPop 3.3.0 (Release Date: NOT OFFICIALLY RELEASED YET)
 * Removed `tryRandomCommit()` from `AbstractGremlinTest`.
 * Changed `gremlin-benchmark` system property for the report location to `benchmarkReportDir` for consistency.
 * Added SysV and systemd init scripts.
+* `valueMap` now returns a `Map<Object,E>` since keys could be `T.id` or `T.label`.


Please make it read GraphTraversal.valueMap().

okram · 2016-11-28T19:36:43Z

docs/src/upgrade/release-3.3.x.asciidoc

@@ -73,6 +73,13 @@ bin/gremlin.sh -e gremlin.groovy

 See: link:https://issues.apache.org/jira/browse/TINKERPOP-1283[TINKERPOP-1283]

+ValueMap


GraphTraversal valueMap() Signature Updated

okram · 2016-11-28T19:37:54Z

...e/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java

@@ -483,7 +483,7 @@
     * @param <E2>         the value type of the returned properties
     * @return the traversal with an appended {@link PropertyMapStep}.
     */
-    public default <E2> GraphTraversal<S, Map<String, E2>> valueMap(final String... propertyKeys) {
+    public default <E2> GraphTraversal<S, Map<Object, E2>> valueMap(final String... propertyKeys) {


Actually, this can still be Map<String,E2>. Its only when you have a boolean "includeTokens" that its Map<Object,Object>.

okram · 2016-11-28T19:39:04Z

I just realized that valueMap() is Map<String,Object>, but valueMap(boolean) is Map<Object,Object>. We should maintain that level of explicitness as I suspect in the future valueMap(boolean) will change given that its sort of a wart right now.

okram · 2016-11-28T20:18:29Z

...ain/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroovyValueMapTest.groovy

@@ -42,5 +42,10 @@ public abstract class GroovyValueMapTest {
        public Traversal<Vertex, Map<String, List<String>>> get_g_VX1X_outXcreatedX_valueMap(final Object v1Id) {
            new ScriptTraversal<>(g, "gremlin-groovy", "g.V(v1Id).out('created').valueMap", "v1Id", v1Id)
        }
+
+        @Override
+        public Traversal<Vertex, Map<Object, Object>> get_g_V_valueMapToken() {


This method is named wrong, it should be:

get_g_V_hasLabelXpersonX_filterXoutEXcreatedXX_valueMapXtrueX()

okram · 2016-11-28T20:19:00Z

...test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ValueMapTest.java

+     */
+    @Test
+    @LoadGraphWith(MODERN)
+    public void valueMapHasObjectKeys() {


This method is named wrong, it should be like the traversal generator method name, minus the get_ prefix.

okram · 2016-11-30T11:30:34Z

This is looking really good. We need one more VOTE.

dkuppitz · 2016-11-30T13:07:04Z

Hold on, I've been busy with other stuff. I will look into this PR today.

dkuppitz · 2016-11-30T16:41:33Z

Looked through the code, did some manual test - all good.

VOTE: +1

Gonna merge it in a few.

valueMap should always return string keys

02a179c

JPMoresmau added 2 commits November 23, 2016 21:54

signature change: valueMap returns Object keys

4af2387

merge master

952c63e

change and upgrade docs

4eba658

fix typo

879003c

okram reviewed Nov 28, 2016

View reviewed changes

only when passing includeTokens are the keys Object

8f038aa

okram reviewed Nov 28, 2016

View reviewed changes

rename test method

02345aa

asfgit merged commit 02345aa into apache:master Nov 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TINKERPOP-1483: valueMap should always return string keys #446

TINKERPOP-1483: valueMap should always return string keys #446

JPMoresmau commented Sep 30, 2016

okram commented Sep 30, 2016

JPMoresmau commented Sep 30, 2016 •

edited

Loading

dkuppitz commented Oct 28, 2016

okram commented Oct 29, 2016

JPMoresmau commented Oct 30, 2016

dkuppitz commented Oct 30, 2016

JPMoresmau commented Oct 30, 2016

dkuppitz commented Oct 30, 2016

robertdale commented Nov 23, 2016

robertdale commented Nov 25, 2016

dkuppitz commented Nov 25, 2016

robertdale commented Nov 26, 2016

okram commented Nov 28, 2016

spmallette commented Nov 28, 2016

okram commented Nov 28, 2016

JPMoresmau commented Nov 28, 2016 •

edited

Loading

okram Nov 28, 2016

okram Nov 28, 2016

okram Nov 28, 2016

okram commented Nov 28, 2016

okram Nov 28, 2016

okram Nov 28, 2016

okram commented Nov 30, 2016

dkuppitz commented Nov 30, 2016

dkuppitz commented Nov 30, 2016

		@@ -73,6 +73,13 @@ bin/gremlin.sh -e gremlin.groovy

		See: link:https://issues.apache.org/jira/browse/TINKERPOP-1283[TINKERPOP-1283]

		ValueMap

TINKERPOP-1483: valueMap should always return string keys #446

TINKERPOP-1483: valueMap should always return string keys #446

Conversation

JPMoresmau commented Sep 30, 2016

okram commented Sep 30, 2016

JPMoresmau commented Sep 30, 2016 • edited Loading

dkuppitz commented Oct 28, 2016

okram commented Oct 29, 2016

JPMoresmau commented Oct 30, 2016

dkuppitz commented Oct 30, 2016

JPMoresmau commented Oct 30, 2016

dkuppitz commented Oct 30, 2016

robertdale commented Nov 23, 2016

robertdale commented Nov 25, 2016

dkuppitz commented Nov 25, 2016

robertdale commented Nov 26, 2016

okram commented Nov 28, 2016

spmallette commented Nov 28, 2016

okram commented Nov 28, 2016

JPMoresmau commented Nov 28, 2016 • edited Loading

okram Nov 28, 2016

Choose a reason for hiding this comment

okram Nov 28, 2016

Choose a reason for hiding this comment

okram Nov 28, 2016

Choose a reason for hiding this comment

okram commented Nov 28, 2016

okram Nov 28, 2016

Choose a reason for hiding this comment

okram Nov 28, 2016

Choose a reason for hiding this comment

okram commented Nov 30, 2016

dkuppitz commented Nov 30, 2016

dkuppitz commented Nov 30, 2016

JPMoresmau commented Sep 30, 2016 •

edited

Loading

JPMoresmau commented Nov 28, 2016 •

edited

Loading