You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by JPMoresmau <gi...@git.apache.org> on 2016/09/30 14:51:13 UTC
[GitHub] tinkerpop pull request #446: TINKERPOP-1483: valueMap should always return s...
GitHub user JPMoresmau opened a pull request:
https://github.com/apache/tinkerpop/pull/446
TINKERPOP-1483: valueMap should always return string keys
Code changed, test added
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/JPMoresmau/tinkerpop TINKERPOP-1483
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/446.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #446
----
commit 02a179ca3aba5089faca25f20647ff7dc1de1e6a
Author: jpmoresmau <jp...@moresmau.fr>
Date: 2016-09-30T14:49:49Z
valueMap should always return string keys
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Thanks for the clarification @dkuppitz
`./docker/build.sh -t -i -n ` passes
VOTE: +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by JPMoresmau <gi...@git.apache.org>.
Github user JPMoresmau commented on the issue:
https://github.com/apache/tinkerpop/pull/446
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #446: TINKERPOP-1483: valueMap should always return s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/446
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/446
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
:smiley:
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`).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Hold on, I've been busy with other stuff. I will look into this PR today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
This is looking really good. We need one more VOTE.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Looked through the code, did some manual test - all good.
VOTE: +1
Gonna merge it in a few.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by JPMoresmau <gi...@git.apache.org>.
Github user JPMoresmau commented on the issue:
https://github.com/apache/tinkerpop/pull/446
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
@dkuppitz -- will you handle merge and thus, do the CHANGELOG and update the upgrade docs on merge?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Can this PR be updated with Map<Object, Object> or should it be closed and open a new one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #446: TINKERPOP-1483: valueMap should always return s...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/446#discussion_r89863961
--- Diff: 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`.
--- End diff --
Please make it read `GraphTraversal.valueMap()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by JPMoresmau <gi...@git.apache.org>.
Github user JPMoresmau commented on the issue:
https://github.com/apache/tinkerpop/pull/446
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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/446
This requires CHANGELOG and upgrade docs too (unless a committer wants to take responsibility for that).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #446: TINKERPOP-1483: valueMap should always return s...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/446#discussion_r89875249
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ValueMapTest.java ---
@@ -125,6 +131,30 @@ public void g_VX1X_outXcreatedX_valueMap() {
}
+ /**
+ * TINKERPOP-1483
+ *
+ */
+ @Test
+ @LoadGraphWith(MODERN)
+ public void valueMapHasObjectKeys() {
--- End diff --
This method is named wrong, it should be like the traversal generator method name, minus the `get_` prefix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
To support something like `valueMap(T.label, "name")`? This would be cool, but would go into another PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Yea, this is a troublesome PR. VOTE -1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #446: TINKERPOP-1483: valueMap should always return s...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/446#discussion_r89875173
--- Diff: gremlin-groovy-test/src/main/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() {
--- End diff --
This method is named wrong, it should be:
```
get_g_V_hasLabelXpersonX_filterXoutEXcreatedXX_valueMapXtrueX()
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Oh, now I see what you're saying. Yes, this would be the right way to go. The current `PropertyMapStep` implementation is pretty weird.
```java
protected Map<String, E> map(...) {
final Map<Object, Object> map = new HashMap<>();
...
return (Map) map;
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...
Posted by JPMoresmau <gi...@git.apache.org>.
Github user JPMoresmau commented on the issue:
https://github.com/apache/tinkerpop/pull/446
I've update the changelog and upgrade doc, not sure it's sufficient, please check! thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---