You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by svanteschubert <gi...@git.apache.org> on 2018/07/15 18:46:30 UTC
[GitHub] tinkerpop pull request #891: TINKERPOP-2006 - Fix for valid GraphML export w...
GitHub user svanteschubert opened a pull request:
https://github.com/apache/tinkerpop/pull/891
TINKERPOP-2006 - Fix for valid GraphML export when graph properties o…
…f a vertex and edge have similar names
I have created an JIRA issue for this patch:
https://issues.apache.org/jira/browse/TINKERPOP-2006
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/svanteschubert/tinkerpop TINKERPOP-2006
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/891.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 #891
----
commit cfc7ecefd370ac438afea03f1e6d718f1eff0fbe
Author: Svante Schubert <sv...@...>
Date: 2018-07-15T18:26:10Z
TINKERPOP-2006 - Fix for valid GraphML export when graph properties of a vertex and edge have similar name
----
---
[GitHub] tinkerpop issue #891: TINKERPOP-2006 - Fix for valid GraphML export when gra...
Posted by svanteschubert <gi...@git.apache.org>.
Github user svanteschubert commented on the issue:
https://github.com/apache/tinkerpop/pull/891
1) Yes, "Collection<String> vertexKeySet.." and same for the edge can still be made final, good catch.
2) No, I did not make any performance test, do you have one for Tinkerpop, what would you suggest?
In any case, I leave it to your taste to adopt it to your desire {#emotions_dlg.wink}
3) There is a key declared in the header of the for loop, what is your suggestion? I do not fully understand your suggestion. Again, I have no strong feelings, in case you like to adopt the source. I only want to remove the reported problem.
4) The scenario comes from creating a graph with Tinkerpop using same-named property on edge and vertex and serializing it to GraphML. A regression test might be the extending an existing Graph example with the color property for an edge and vertex, which would be the correct location/class to create in Tinkerpop such a regression test?
---
[GitHub] tinkerpop pull request #891: TINKERPOP-2006 - Fix for valid GraphML export w...
Posted by twilmes <gi...@git.apache.org>.
Github user twilmes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/891#discussion_r204067177
--- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphml/GraphMLWriter.java ---
@@ -218,20 +220,33 @@ private void writeTypes(final Map<String, String> identifiedVertexKeyTypes,
final Map<String, String> identifiedEdgeKeyTypes,
final XMLStreamWriter writer) throws XMLStreamException {
// <key id="weight" for="edge" attr.name="weight" attr.type="float"/>
- final Collection<String> vertexKeySet = getVertexKeysAndNormalizeIfRequired(identifiedVertexKeyTypes);
+ Collection<String> vertexKeySet = getVertexKeysAndNormalizeIfRequired(identifiedVertexKeyTypes);
+ Collection<String> edgeKeySet = getEdgeKeysAndNormalizeIfRequired(identifiedEdgeKeyTypes);
+ // in case vertex and edge may have the same attribute name, the key id in graphml have to be different
+ intersection = CollectionUtils.intersection(vertexKeySet, edgeKeySet);
+ // speeding-up later checks
+ if(intersection.isEmpty()){
+ intersection = null;
+ }
for (String key : vertexKeySet) {
writer.writeStartElement(GraphMLTokens.KEY);
- writer.writeAttribute(GraphMLTokens.ID, key);
+ if(intersection != null && intersection.contains(key)){
--- End diff --
Maybe here and below you could set a key variable once, something like:
```
String keyVal = key;
if (!intersection.isEmpty() && intersection.contains(key)) {
keyVal = key.concat(GraphMLTokens.VERTEX_SUFFIX)
}
writer.writeAttribute(GraphMLTokens.ID, key);
```
---
[GitHub] tinkerpop issue #891: TINKERPOP-2006 - Fix for valid GraphML export when gra...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/891
@svanteschubert do you plan to come back to this one and #892 ? I think that if you address @twilmes issues here we could push forward at least this change and get it merged. Also, if you are coming back to this, I think we'd want to see some tests for this change somehow. None of the toy graphs have shared edge/vertex property keys that I can think of, so you might need special test graph data for this.
---
[GitHub] tinkerpop pull request #891: TINKERPOP-2006 - Fix for valid GraphML export w...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/891
---
[GitHub] tinkerpop pull request #891: TINKERPOP-2006 - Fix for valid GraphML export w...
Posted by twilmes <gi...@git.apache.org>.
Github user twilmes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/891#discussion_r204065865
--- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphml/GraphMLWriter.java ---
@@ -218,20 +220,33 @@ private void writeTypes(final Map<String, String> identifiedVertexKeyTypes,
final Map<String, String> identifiedEdgeKeyTypes,
final XMLStreamWriter writer) throws XMLStreamException {
// <key id="weight" for="edge" attr.name="weight" attr.type="float"/>
- final Collection<String> vertexKeySet = getVertexKeysAndNormalizeIfRequired(identifiedVertexKeyTypes);
+ Collection<String> vertexKeySet = getVertexKeysAndNormalizeIfRequired(identifiedVertexKeyTypes);
+ Collection<String> edgeKeySet = getEdgeKeysAndNormalizeIfRequired(identifiedEdgeKeyTypes);
+ // in case vertex and edge may have the same attribute name, the key id in graphml have to be different
+ intersection = CollectionUtils.intersection(vertexKeySet, edgeKeySet);
+ // speeding-up later checks
+ if(intersection.isEmpty()){
--- End diff --
I can see what you're doing for the `null` check but I'm wondering if it's worth it versus just initializing the intersection to an empty set at the start to make the code a bit simpler. Did you do some performance comparison and identify that the null check approach was a lot faster?
---
[GitHub] tinkerpop issue #891: TINKERPOP-2006 - Fix for valid GraphML export when gra...
Posted by svanteschubert <gi...@git.apache.org>.
Github user svanteschubert commented on the issue:
https://github.com/apache/tinkerpop/pull/891
What is the correct GitHub / Tinkerpop procedure to update a patch? Do you have a pointer?
I decided to start from scratch and cloned a fresh Tinkerpop repro, to start a fresh git branch from the latest sources.
But the master has build problems on my machine with the latest patches on Windows (JDK 181).
Do you build as well on Windows? Going to reboot here.. ;-)
---
[GitHub] tinkerpop pull request #891: TINKERPOP-2006 - Fix for valid GraphML export w...
Posted by twilmes <gi...@git.apache.org>.
Github user twilmes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/891#discussion_r204065136
--- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphml/GraphMLWriter.java ---
@@ -218,20 +220,33 @@ private void writeTypes(final Map<String, String> identifiedVertexKeyTypes,
final Map<String, String> identifiedEdgeKeyTypes,
final XMLStreamWriter writer) throws XMLStreamException {
// <key id="weight" for="edge" attr.name="weight" attr.type="float"/>
- final Collection<String> vertexKeySet = getVertexKeysAndNormalizeIfRequired(identifiedVertexKeyTypes);
+ Collection<String> vertexKeySet = getVertexKeysAndNormalizeIfRequired(identifiedVertexKeyTypes);
--- End diff --
Could these still be made `final`?
---
[GitHub] tinkerpop issue #891: TINKERPOP-2006 - Fix for valid GraphML export when gra...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/891
oh - and if your build is failing on the `gremlin-python` module then that is a known issue at this point. i only just fixed my local build and i'm still not sure how i did that. feel free to ignore failures in that module.
---
[GitHub] tinkerpop issue #891: TINKERPOP-2006 - Fix for valid GraphML export when gra...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/891
you should have been able to just keep pushing commits to the same branch you had and they would have reflected here in this pull request.
if you destroyed your repo and re-cloned then i think you will have to close this pull request and open a fresh one. if you want this particular commit as a starting point you should be able to still:
`git fetch upstream pull/891/head:pr-891`
where `upstream` is this repo and then cherry-pick the commit. i assume that will work.
---