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/08/02 14:03:18 UTC
[GitHub] tinkerpop pull request #905: Pr 891
GitHub user svanteschubert opened a pull request:
https://github.com/apache/tinkerpop/pull/905
Pr 891
I have added a test and adjusted the changed according to the comments.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/svanteschubert/tinkerpop pr-891
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/905.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 #905
----
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
commit 41f2ab8a5cd6bdb60a195489fce38ef38baaf232
Author: Svante Schubert <sv...@...>
Date: 2018-08-02T13:33:04Z
TINKERPOP-2006 Fixing GraphML serialization with similar named property for edge and vertex
----
---
[GitHub] tinkerpop issue #905: Pr 891
Posted by svanteschubert <gi...@git.apache.org>.
Github user svanteschubert commented on the issue:
https://github.com/apache/tinkerpop/pull/905
I am indeed a little under pressure, currently on a business trip to Warschau. I might be able to spend time end of next week. But would appreciate it, if you could take over.. Simply bad timing.. :)
Thanks Svante
---
[GitHub] tinkerpop pull request #905: Pr 891
Posted by svanteschubert <gi...@git.apache.org>.
Github user svanteschubert commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/905#discussion_r207240077
--- Diff: gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/structure/io/graphml/tinkerpop-classic-normalized.xml ---
@@ -1,15 +1,18 @@
<?xml version="1.0" ?>
<graphml xmlns="http://graphml.graphdrawing.org/xmlns" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://graphml.graphdrawing.org/xmlns http://graphml.graphdrawing.org/xmlns/1.1/graphml.xsd">
<key id="age" for="node" attr.name="age" attr.type="int"></key>
+ <key id="colorV" for="node" attr.name="color" attr.type="string"></key>
--- End diff --
Well. than perhaps rename it and place it aside?
The problem being solved is to add two properties on edge and vertex having the same name during run-time and serialize this new graph to GraphML.
---
[GitHub] tinkerpop pull request #905: Pr 891
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/905#discussion_r207239273
--- Diff: gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/structure/io/graphml/tinkerpop-classic-normalized.xml ---
@@ -1,15 +1,18 @@
<?xml version="1.0" ?>
<graphml xmlns="http://graphml.graphdrawing.org/xmlns" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://graphml.graphdrawing.org/xmlns http://graphml.graphdrawing.org/xmlns/1.1/graphml.xsd">
<key id="age" for="node" attr.name="age" attr.type="int"></key>
+ <key id="colorV" for="node" attr.name="color" attr.type="string"></key>
--- End diff --
This is a standard dataset. We can't change the data within it.
---
[GitHub] tinkerpop pull request #905: Pr 891
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/905#discussion_r209302881
--- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphml/GraphMLWriter.java ---
@@ -18,6 +18,21 @@
*/
package org.apache.tinkerpop.gremlin.structure.io.graphml;
+import java.io.IOException;
--- End diff --
i just happened to peek back in on this PR and noticed that it looks like your IDE re-organized the imports a bit. when you come back to this could you please revert that portion of the change?
---
[GitHub] tinkerpop pull request #905: Pr 891
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/905#discussion_r207705128
--- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphml/GraphMLWriter.java ---
@@ -212,26 +214,39 @@ private XMLStreamWriter configureWriter(final OutputStream outputStream) throws
return writer;
} else
return utf8Writer;
- }
+ }
private void writeTypes(final Map<String, String> identifiedVertexKeyTypes,
- final Map<String, String> identifiedEdgeKeyTypes,
- final XMLStreamWriter writer) throws XMLStreamException {
+ 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);
+ final 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 like the suggestion @twilmes had for this here:
https://github.com/apache/tinkerpop/pull/891#pullrequestreview-139083855
it sounds like a more fluent way to handle this logic.
---
[GitHub] tinkerpop issue #905: Pr 891
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/905
@svanteschubert i'm just checking in - do you think you will have time to come back to this pull request to address the comments presented? I'm not trying to rush you, but if you're really busy i can probably look to merge this as-is and make the adjustments myself, if you'd like.
---
[GitHub] tinkerpop issue #905: Pr 891
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/905
Ok - I handled the merge and polished up a couple of things. Thanks for your help on this. Take care!
---
[GitHub] tinkerpop pull request #905: Pr 891
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/905#discussion_r207241886
--- Diff: gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/structure/io/graphml/tinkerpop-classic-normalized.xml ---
@@ -1,15 +1,18 @@
<?xml version="1.0" ?>
<graphml xmlns="http://graphml.graphdrawing.org/xmlns" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://graphml.graphdrawing.org/xmlns http://graphml.graphdrawing.org/xmlns/1.1/graphml.xsd">
<key id="age" for="node" attr.name="age" attr.type="int"></key>
+ <key id="colorV" for="node" attr.name="color" attr.type="string"></key>
--- End diff --
I think you'd missed my comment on the JIRA about this - please add a test here:
https://github.com/apache/tinkerpop/blob/e3018fb2e98df62a21272446f63cea5ee550408e/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/io/IoTest.java#L108
then , since it is a one-off test just for GraphML, I think you can just embed a small GraphML file as a String right in the test.
---
[GitHub] tinkerpop pull request #905: Pr 891
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/905
---