Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1520

Difference between 'has' step generated graphson2.0 in java and python glv implementation

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.3
    • Fix Version/s: 3.3.0
    • Component/s: python
    • Labels:
      None

      Description

      Noticed that between the java and python implementations, the graphson2.0 payload generated for a has step is different. i.e. for the given traversal:

      g.E().has("weight", 0.2)

      The java implementation produces the following graphson:

      {"@type":"g:Bytecode","@value":{"step":[["E"],["has","weight",{"@type":"g:P","@value":{"predicate":"eq","value":{"@type":"g:Double","@value":0.2}}}]]}}
      

      where the python implementation produces the following:

       {"@type":"g:Bytecode","@value":{"step":[["E"],["has","weight",0.2]]}}
      

      In the java case, a g:P typed (predicate) value is provided, where in the python case that isn't the case.

      I'm assuming the java one is correct (primarily since the graph backend seems to like it and return the expected result). Should GLV implementations behave this way? I noticed that GraphTraversal#has(String propertyKey, Object value) in the java TinkerPop api wraps the value in a predicate (P.eq) under the covers (link) so maybe implementors will need to do the same (python link)?

        Issue Links

          Activity

          Hide
          andrew.tolbert Andy Tolbert added a comment - - edited

          Just to correct my previous comment. The query appears to work correctly in both cases (whether a predicate is used or not), but would still be good for the behavior to be consistent across implementations.

          Show
          andrew.tolbert Andy Tolbert added a comment - - edited Just to correct my previous comment. The query appears to work correctly in both cases (whether a predicate is used or not), but would still be good for the behavior to be consistent across implementations.
          Hide
          okram Marko A. Rodriguez added a comment -

          This ticket has spawned a beautiful body of tests in the TINKERPOP-1520 branch. tp32/ PR by end of day tomorrow.

          Show
          okram Marko A. Rodriguez added a comment - This ticket has spawned a beautiful body of tests in the TINKERPOP-1520 branch. tp32/ PR by end of day tomorrow.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user okram opened a pull request:

          https://github.com/apache/tinkerpop/pull/499

          TINKERPOP-1520: Difference between 'has' step generated graphson2.0 in java and python glv implementation

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

          This branch has done alot around testing and GraphSON.

          • We now verify that all translators create Traversals with bytecode identical to the original translated bytecode. (tested against all traversals in ProcessSuite)
          • We now verify that Gremlin-Python's GraphSON Bytecode representation is identical to Gremlin-Java's. (tested against all traversals in ProcessSuite)
          • We now verify that GraphTraversal is in one-to-one correspondence with Bytecode. That is, has("name") -> [has,name]. (bad ass reflection technique used here)
          • There was a GraphSON 2.0 issue with Edge properties. Changed to the same format a VertexProperties. Smaller footprint and only necessary information.
          • Added VertexID ("vertex"-field) to VertexProperty in GraphSON 2.0 – how else will you attach?
          • Added Element ("element"-field) to Property in GraphSON 2.0 – how else will you attach?
          • Added TraversalStrategy classes to GryoMapper as we (@spmallette !!! shame on you) simply dropped `withStrategies()` instructions in Gryo Bytecode serialization.
          • TranslationStrategy is now smart to remove itself from the Bytecode. No longer do the Translators have to be wary of this (lots of code deleted).
          • We now verify that Gryo created bytecode is identical to source bytecode.

          ...need I go on?

          VOTE +1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/tinkerpop TINKERPOP-1520

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/499.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 #499


          commit 6aa6fc5adc47170e1c049bc47970bcaaf54b5ff2
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-11-16T23:08:19Z

          TranslationStrategy (a test framework strategy) now verifies that the Bytecode submitted to the translator is equal to the bytecode of the generated/translated traversal. This was a nightmare to get everything working. We had so many little nick nack bugs. In particular, VertexProperty and Property GraphSON serialization needs to have their element() serialized or else you can't attach/argument. This is still a problem in Gryo and we will need to solve it for 3.3.0. In short, lots of test cases added, lots of tweaks to the various translators made, graphson.py updated significantly (though backwards compatible), etc. Crazy shiiiiiiiet.

          commit 86e420f4c0e2ef9d163c87b3540ed2058193520e
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-11-17T19:16:58Z

          Element data in VertexProperty and Property are now minimilistic and only what is necessay to attach the Property/VertexProperty. Added more test cases accordingly.

          commit 09bde87e57eb77da7806724a9cbf06181b97f040
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-11-17T20:47:02Z

          changed the Edge property GraphSON serialization to do key/value map like VertexProperty metaProperties. Talking with @spmallette – we believe the Edge property serialization is a bug. Other random cleanups.

          commit e86372d0733088a2bdd87a0827eaa7abdefcf141
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-11-18T00:58:32Z

          I just did Gods work. TranslationStrategy is now smart about removing itself from the Bytecode so all the Translators no longer have to have IS_TESTING in them. GraphTraversalTest has a sweet method that uses reflection to generate fake arguments to ensure that the Bytecode instruction arguments are identical to the method arguments. Found lots of redirects in GraphTraversal that have since been fixed. Step->Instruction are in one-to-one correspondence. More work on GraphSON and added more tests around serialization in both Python and Java. Added an IS_TESTING extension to PythonGraphSONTranslator that ensures that the GraphSON representaiton of the java Bytecode is the same as the Python bytecode. TaDa. Added some more cool helper methods to BytecodeHelper. Crazy cool stuff.

          commit 31cdb4bf6cfb02e56cce35e536d85c6c13b03e97
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-11-18T13:42:41Z

          IoPropertyTest had to be tweaked cause graphson v2 without embedded types was turning Neo4j ids to ints instead of longs. We really should get rid of graphson v2-noembedding (@spmallette). Finally, along with doing string comparison of the GraphSON from the bytecode of both Java and Python, I also convert to a Map<String,Object> and test the .equals() of the two. Done with this branch. What a whirl wind tour.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user okram opened a pull request: https://github.com/apache/tinkerpop/pull/499 TINKERPOP-1520 : Difference between 'has' step generated graphson2.0 in java and python glv implementation https://issues.apache.org/jira/browse/TINKERPOP-1520 This branch has done alot around testing and GraphSON. We now verify that all translators create Traversals with bytecode identical to the original translated bytecode. (tested against all traversals in ProcessSuite) We now verify that Gremlin-Python's GraphSON Bytecode representation is identical to Gremlin-Java's. (tested against all traversals in ProcessSuite) We now verify that GraphTraversal is in one-to-one correspondence with Bytecode. That is, has("name") -> [has,name] . (bad ass reflection technique used here) There was a GraphSON 2.0 issue with Edge properties. Changed to the same format a VertexProperties. Smaller footprint and only necessary information. Added VertexID ("vertex"-field) to VertexProperty in GraphSON 2.0 – how else will you attach? Added Element ("element"-field) to Property in GraphSON 2.0 – how else will you attach? Added TraversalStrategy classes to GryoMapper as we (@spmallette !!! shame on you) simply dropped `withStrategies()` instructions in Gryo Bytecode serialization. TranslationStrategy is now smart to remove itself from the Bytecode. No longer do the Translators have to be wary of this (lots of code deleted). We now verify that Gryo created bytecode is identical to source bytecode. ...need I go on? VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1520 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/499.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 #499 commit 6aa6fc5adc47170e1c049bc47970bcaaf54b5ff2 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-11-16T23:08:19Z TranslationStrategy (a test framework strategy) now verifies that the Bytecode submitted to the translator is equal to the bytecode of the generated/translated traversal. This was a nightmare to get everything working. We had so many little nick nack bugs. In particular, VertexProperty and Property GraphSON serialization needs to have their element() serialized or else you can't attach/argument. This is still a problem in Gryo and we will need to solve it for 3.3.0. In short, lots of test cases added, lots of tweaks to the various translators made, graphson.py updated significantly (though backwards compatible), etc. Crazy shiiiiiiiet. commit 86e420f4c0e2ef9d163c87b3540ed2058193520e Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-11-17T19:16:58Z Element data in VertexProperty and Property are now minimilistic and only what is necessay to attach the Property/VertexProperty. Added more test cases accordingly. commit 09bde87e57eb77da7806724a9cbf06181b97f040 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-11-17T20:47:02Z changed the Edge property GraphSON serialization to do key/value map like VertexProperty metaProperties. Talking with @spmallette – we believe the Edge property serialization is a bug. Other random cleanups. commit e86372d0733088a2bdd87a0827eaa7abdefcf141 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-11-18T00:58:32Z I just did Gods work. TranslationStrategy is now smart about removing itself from the Bytecode so all the Translators no longer have to have IS_TESTING in them. GraphTraversalTest has a sweet method that uses reflection to generate fake arguments to ensure that the Bytecode instruction arguments are identical to the method arguments. Found lots of redirects in GraphTraversal that have since been fixed. Step->Instruction are in one-to-one correspondence. More work on GraphSON and added more tests around serialization in both Python and Java. Added an IS_TESTING extension to PythonGraphSONTranslator that ensures that the GraphSON representaiton of the java Bytecode is the same as the Python bytecode. TaDa. Added some more cool helper methods to BytecodeHelper. Crazy cool stuff. commit 31cdb4bf6cfb02e56cce35e536d85c6c13b03e97 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-11-18T13:42:41Z IoPropertyTest had to be tweaked cause graphson v2 without embedded types was turning Neo4j ids to ints instead of longs. We really should get rid of graphson v2-noembedding (@spmallette). Finally, along with doing string comparison of the GraphSON from the bytecode of both Java and Python, I also convert to a Map<String,Object> and test the .equals() of the two. Done with this branch. What a whirl wind tour.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          Oh – and I forgot. Gremlin-Python now has serializers for Vertex, Edge, Property, VertexProperty... Thus, you can `g.V(v[1])` from Gremlin-Python to Gremlin-Java.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 Oh – and I forgot. Gremlin-Python now has serializers for Vertex, Edge, Property, VertexProperty... Thus, you can `g.V(v [1] )` from Gremlin-Python to Gremlin-Java.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88705921

          — Diff: CHANGELOG.asciidoc —
          @@ -26,6 +26,12 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
          TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET)
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

          +* Added "attachment requisite" `VertexProperty.element()` and `Property.element()` data in GraphSON serialization.
          +* Added `Vertex`, `Edge`, `VertexProperty`, and `Property` serializers to Gremlin-Python and exposed tests that use graph object arguments.
          +* `Bytecode.getSourceInstructions()` and `Bytecode.getStepInstructions()` now returns `List<Instruction>` instead of `Iterable<Instruction>`.
          +* Added various `TraversalStrategy` registrations with `GryoMapper`.
          +* Fixed a naming mistake in Gremlin-Python: `IdentityRemoveStrategy` is now called `IdentityRemovalStrategy`. (breaking)
          — End diff –

          I've noticed that in the last few versions you've started adding "breaking" to changelog entries. I've been tying "breaking" to tickets that will then show up in the changelog on release. We can start doing this from now i guess, but it's not been done consistently in changelog in any way. I guess that's a really minor breaking change which wouldn't warrant a separate ticket. I dunno - do we want to keep doing this and should it be done consistently going forward?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88705921 — Diff: CHANGELOG.asciidoc — @@ -26,6 +26,12 @@ image:: https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Added "attachment requisite" `VertexProperty.element()` and `Property.element()` data in GraphSON serialization. +* Added `Vertex`, `Edge`, `VertexProperty`, and `Property` serializers to Gremlin-Python and exposed tests that use graph object arguments. +* `Bytecode.getSourceInstructions()` and `Bytecode.getStepInstructions()` now returns `List<Instruction>` instead of `Iterable<Instruction>`. +* Added various `TraversalStrategy` registrations with `GryoMapper`. +* Fixed a naming mistake in Gremlin-Python: `IdentityRemoveStrategy` is now called `IdentityRemovalStrategy`. ( breaking ) — End diff – I've noticed that in the last few versions you've started adding "breaking" to changelog entries. I've been tying "breaking" to tickets that will then show up in the changelog on release. We can start doing this from now i guess, but it's not been done consistently in changelog in any way. I guess that's a really minor breaking change which wouldn't warrant a separate ticket. I dunno - do we want to keep doing this and should it be done consistently going forward?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          This is a nice body of work. So you also close this one with this PR?

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

          Any others?

          As there are some breaking changes to graphson formatting, can you please add upgrade docs? also, the io documentation needs to be updated and regenerated.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/499 This is a nice body of work. So you also close this one with this PR? https://issues.apache.org/jira/browse/TINKERPOP-1454 Any others? As there are some breaking changes to graphson formatting, can you please add upgrade docs? also, the io documentation needs to be updated and regenerated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88707852

          — Diff: CHANGELOG.asciidoc —
          @@ -26,6 +26,12 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
          TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET)
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

          +* Added "attachment requisite" `VertexProperty.element()` and `Property.element()` data in GraphSON serialization.
          +* Added `Vertex`, `Edge`, `VertexProperty`, and `Property` serializers to Gremlin-Python and exposed tests that use graph object arguments.
          +* `Bytecode.getSourceInstructions()` and `Bytecode.getStepInstructions()` now returns `List<Instruction>` instead of `Iterable<Instruction>`.
          +* Added various `TraversalStrategy` registrations with `GryoMapper`.
          +* Fixed a naming mistake in Gremlin-Python: `IdentityRemoveStrategy` is now called `IdentityRemovalStrategy`. (breaking)
          — End diff –

          If you don't like it, lets not do it. I can remove the "breaking" tag in the CHANGELOG on merge.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88707852 — Diff: CHANGELOG.asciidoc — @@ -26,6 +26,12 @@ image:: https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Added "attachment requisite" `VertexProperty.element()` and `Property.element()` data in GraphSON serialization. +* Added `Vertex`, `Edge`, `VertexProperty`, and `Property` serializers to Gremlin-Python and exposed tests that use graph object arguments. +* `Bytecode.getSourceInstructions()` and `Bytecode.getStepInstructions()` now returns `List<Instruction>` instead of `Iterable<Instruction>`. +* Added various `TraversalStrategy` registrations with `GryoMapper`. +* Fixed a naming mistake in Gremlin-Python: `IdentityRemoveStrategy` is now called `IdentityRemovalStrategy`. ( breaking ) — End diff – If you don't like it, lets not do it. I can remove the "breaking" tag in the CHANGELOG on merge.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88708033

          — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java —
          @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() {
          }
          }
          }
          +
          + @Test
          + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception {
          + final Random random = new Random();
          — End diff –

          Lets keep it un-seeded. If it breaks down the road, we learn something. If not, then its good.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88708033 — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java — @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() { } } } + + @Test + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception { + final Random random = new Random(); — End diff – Lets keep it un-seeded. If it breaks down the road, we learn something. If not, then its good.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88708180

          — Diff: CHANGELOG.asciidoc —
          @@ -26,6 +26,12 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
          TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET)
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

          +* Added "attachment requisite" `VertexProperty.element()` and `Property.element()` data in GraphSON serialization.
          +* Added `Vertex`, `Edge`, `VertexProperty`, and `Property` serializers to Gremlin-Python and exposed tests that use graph object arguments.
          +* `Bytecode.getSourceInstructions()` and `Bytecode.getStepInstructions()` now returns `List<Instruction>` instead of `Iterable<Instruction>`.
          +* Added various `TraversalStrategy` registrations with `GryoMapper`.
          +* Fixed a naming mistake in Gremlin-Python: `IdentityRemoveStrategy` is now called `IdentityRemovalStrategy`. (breaking)
          — End diff –

          mostly don't like it because we didn't do it from the start....my ocd is nagging at me about it now and there's no easy way to go back through and retroactively fix it. if you're cool to drop them, i'm fine with that.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88708180 — Diff: CHANGELOG.asciidoc — @@ -26,6 +26,12 @@ image:: https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Added "attachment requisite" `VertexProperty.element()` and `Property.element()` data in GraphSON serialization. +* Added `Vertex`, `Edge`, `VertexProperty`, and `Property` serializers to Gremlin-Python and exposed tests that use graph object arguments. +* `Bytecode.getSourceInstructions()` and `Bytecode.getStepInstructions()` now returns `List<Instruction>` instead of `Iterable<Instruction>`. +* Added various `TraversalStrategy` registrations with `GryoMapper`. +* Fixed a naming mistake in Gremlin-Python: `IdentityRemoveStrategy` is now called `IdentityRemovalStrategy`. ( breaking ) — End diff – mostly don't like it because we didn't do it from the start....my ocd is nagging at me about it now and there's no easy way to go back through and retroactively fix it. if you're cool to drop them, i'm fine with that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88708851

          — Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslator.java —
          @@ -47,8 +49,6 @@
          */
          public final class GroovyTranslator implements Translator.ScriptTranslator {

          • private static final boolean IS_TESTING = Boolean.valueOf(System.getProperty("is.testing", "false"));
              • End diff –

          No. Its just got gutted from all the `XXXTranslators` as they needed to remove the `[withStrategies, TranslatorStrategy]` instruction from the bytecode before compiling. I realized, `TranslationStrategy` can just remove itself from the bytecode before sending the bytecode to the `Translator`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88708851 — Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslator.java — @@ -47,8 +49,6 @@ */ public final class GroovyTranslator implements Translator.ScriptTranslator { private static final boolean IS_TESTING = Boolean.valueOf(System.getProperty("is.testing", "false")); End diff – No. Its just got gutted from all the `XXXTranslators` as they needed to remove the ` [withStrategies, TranslatorStrategy] ` instruction from the bytecode before compiling. I realized, `TranslationStrategy` can just remove itself from the bytecode before sending the bytecode to the `Translator`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          Yea. I will do the upgrade and io docs. Integration testing Giraph is running right now :| ... once that completes, I will push doc updates.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 Yea. I will do the upgrade and io docs. Integration testing Giraph is running right now :| ... once that completes, I will push doc updates.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88709124

          — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java —
          @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() {
          }
          }
          }
          +
          + @Test
          + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception {
          + final Random random = new Random();
          — End diff –

          Ok - I have a body of tests that are kinda like this. I add this:

          https://github.com/apache/tinkerpop/blob/master/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java#L375-L379

          so that they only run when `-DassertNonDeterministic` is added to the `mvn clean install`. This should probably be one of those tests.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88709124 — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java — @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() { } } } + + @Test + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception { + final Random random = new Random(); — End diff – Ok - I have a body of tests that are kinda like this. I add this: https://github.com/apache/tinkerpop/blob/master/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java#L375-L379 so that they only run when `-DassertNonDeterministic` is added to the `mvn clean install`. This should probably be one of those tests.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          Indeed there seems to be some breaking changes in the protocol. It seems ok for pure TinkerPop since the parsing code is updated accordingly but will cause problems to implementors, just as a note (maybe only for my own remembering)...

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/499 Indeed there seems to be some breaking changes in the protocol. It seems ok for pure TinkerPop since the parsing code is updated accordingly but will cause problems to implementors, just as a note (maybe only for my own remembering)...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88769301

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -175,8 +177,30 @@ public PropertyJacksonSerializer() {
          public void serialize(final Property property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider)
          throws IOException {
          jsonGenerator.writeStartObject();

          • jsonGenerator.writeObjectField(GraphSONTokens.KEY, property.key());
            + jsonGenerator.writeStringField(GraphSONTokens.KEY, property.key());
            jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value());
            + if (property.element() instanceof VertexProperty) {
            + VertexProperty vertexProperty = (VertexProperty) property.element();
            + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT);
            + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:VertexProperty");
              • End diff –

          It isn't great to bypass the typing system.. As the type system is dynamic, hardcoding the type can lead to inconsistent types if users overrides the Gremlin standard serializers and types as it is currently possible. But I see why we can't just jsonGenerator.writeObjectField(GraphSONTokens.ELEMENT, property.element()), because that would, well, re-write VertexProperty's properties and so on. We can maybe introduce a "g:Element" type? Or maybe just write a simple Map here where the deserializers are aware of what to do with a "element" field? wdyt?

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88769301 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -175,8 +177,30 @@ public PropertyJacksonSerializer() { public void serialize(final Property property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider) throws IOException { jsonGenerator.writeStartObject(); jsonGenerator.writeObjectField(GraphSONTokens.KEY, property.key()); + jsonGenerator.writeStringField(GraphSONTokens.KEY, property.key()); jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value()); + if (property.element() instanceof VertexProperty) { + VertexProperty vertexProperty = (VertexProperty) property.element(); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT); + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:VertexProperty"); End diff – It isn't great to bypass the typing system.. As the type system is dynamic, hardcoding the type can lead to inconsistent types if users overrides the Gremlin standard serializers and types as it is currently possible. But I see why we can't just jsonGenerator.writeObjectField(GraphSONTokens.ELEMENT, property.element()), because that would, well, re-write VertexProperty's properties and so on. We can maybe introduce a "g:Element" type? Or maybe just write a simple Map here where the deserializers are aware of what to do with a "element" field? wdyt?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88769492

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -175,8 +177,30 @@ public PropertyJacksonSerializer() {
          public void serialize(final Property property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider)
          throws IOException {
          jsonGenerator.writeStartObject();

          • jsonGenerator.writeObjectField(GraphSONTokens.KEY, property.key());
            + jsonGenerator.writeStringField(GraphSONTokens.KEY, property.key());
            jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value());
            + if (property.element() instanceof VertexProperty) { + VertexProperty vertexProperty = (VertexProperty) property.element(); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT); + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:VertexProperty"); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.VALUEPROP); + jsonGenerator.writeObjectField(GraphSONTokens.ID, vertexProperty.id()); + jsonGenerator.writeStringField(GraphSONTokens.LABEL, vertexProperty.label()); + jsonGenerator.writeObjectField(GraphSONTokens.VERTEX, vertexProperty.element().id()); + jsonGenerator.writeEndObject(); + jsonGenerator.writeEndObject(); + }

            else if (property.element() instanceof Edge) {
            + Edge edge = (Edge) property.element();
            + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT);
            + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:Edge");

              • End diff –

          This will write an object of type "g:Edge" but will not contain the same content than what a Edge serializer would have serialized, and so the contents are inconsistent. Here what is missing is the out/inVLabel

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88769492 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -175,8 +177,30 @@ public PropertyJacksonSerializer() { public void serialize(final Property property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider) throws IOException { jsonGenerator.writeStartObject(); jsonGenerator.writeObjectField(GraphSONTokens.KEY, property.key()); + jsonGenerator.writeStringField(GraphSONTokens.KEY, property.key()); jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value()); + if (property.element() instanceof VertexProperty) { + VertexProperty vertexProperty = (VertexProperty) property.element(); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT); + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:VertexProperty"); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.VALUEPROP); + jsonGenerator.writeObjectField(GraphSONTokens.ID, vertexProperty.id()); + jsonGenerator.writeStringField(GraphSONTokens.LABEL, vertexProperty.label()); + jsonGenerator.writeObjectField(GraphSONTokens.VERTEX, vertexProperty.element().id()); + jsonGenerator.writeEndObject(); + jsonGenerator.writeEndObject(); + } else if (property.element() instanceof Edge) { + Edge edge = (Edge) property.element(); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT); + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:Edge"); End diff – This will write an object of type "g:Edge" but will not contain the same content than what a Edge serializer would have serialized, and so the contents are inconsistent. Here what is missing is the out/inVLabel
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88769639

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -193,44 +217,46 @@ public VertexPropertyJacksonSerializer(final boolean normalize, final boolean in
          }

          @Override

          • public void serialize(final VertexProperty property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider)
            + public void serialize(final VertexProperty vertexProperty, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider)
            throws IOException {
            jsonGenerator.writeStartObject();
          • jsonGenerator.writeObjectField(GraphSONTokens.ID, property.id());
          • jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value());
          • if (includeLabel)
          • jsonGenerator.writeStringField(GraphSONTokens.LABEL, property.label());
          • tryWriteMetaProperties(property, jsonGenerator, normalize);
            + jsonGenerator.writeObjectField(GraphSONTokens.ID, vertexProperty.id());
            + jsonGenerator.writeObjectField(GraphSONTokens.VALUE, vertexProperty.value());
            + if (null != vertexProperty.element())
            + jsonGenerator.writeObjectField(GraphSONTokens.VERTEX, vertexProperty.element().id());
              • End diff –

          Same as https://github.com/apache/tinkerpop/pull/499/files#r88769610

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88769639 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -193,44 +217,46 @@ public VertexPropertyJacksonSerializer(final boolean normalize, final boolean in } @Override public void serialize(final VertexProperty property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider) + public void serialize(final VertexProperty vertexProperty, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider) throws IOException { jsonGenerator.writeStartObject(); jsonGenerator.writeObjectField(GraphSONTokens.ID, property.id()); jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value()); if (includeLabel) jsonGenerator.writeStringField(GraphSONTokens.LABEL, property.label()); tryWriteMetaProperties(property, jsonGenerator, normalize); + jsonGenerator.writeObjectField(GraphSONTokens.ID, vertexProperty.id()); + jsonGenerator.writeObjectField(GraphSONTokens.VALUE, vertexProperty.value()); + if (null != vertexProperty.element()) + jsonGenerator.writeObjectField(GraphSONTokens.VERTEX, vertexProperty.element().id()); End diff – Same as https://github.com/apache/tinkerpop/pull/499/files#r88769610
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88769838

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -299,6 +325,7 @@ public void serialize(final TraversalExplanation traversalExplanation, final Jso
          intermediate.put(GraphSONTokens.STRATEGY, pair.getValue0().toString());
          intermediate.put(GraphSONTokens.CATEGORY, pair.getValue0().getTraversalCategory().getSimpleName());
          intermediate.put(GraphSONTokens.TRAVERSAL, getStepsAsList(pair.getValue1()));
          + intermediate.put(GraphSONTokens.TRAVERSAL, getStepsAsList(pair.getValue1()));
          — End diff –

          Is this line supposed to be duplicated?

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88769838 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -299,6 +325,7 @@ public void serialize(final TraversalExplanation traversalExplanation, final Jso intermediate.put(GraphSONTokens.STRATEGY, pair.getValue0().toString()); intermediate.put(GraphSONTokens.CATEGORY, pair.getValue0().getTraversalCategory().getSimpleName()); intermediate.put(GraphSONTokens.TRAVERSAL, getStepsAsList(pair.getValue1())); + intermediate.put(GraphSONTokens.TRAVERSAL, getStepsAsList(pair.getValue1())); — End diff – Is this line supposed to be duplicated?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88770063

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -459,9 +486,33 @@ public PropertyJacksonDeserializer() {

          @Override
          public Property createObject(final Map<String, Object> propData)

          { - return new DetachedProperty( - (String) propData.get(GraphSONTokens.KEY), - propData.get(GraphSONTokens.VALUE)); + final Object element = propData.get(GraphSONTokens.ELEMENT); + return element instanceof Element ? // graphson-non-embedded is treated differently, but since this is a hard coded embedding... + new DetachedProperty<>((String) propData.get(GraphSONTokens.KEY), propData.get(GraphSONTokens.VALUE), (Element) element) : + new DetachedProperty<>((String) propData.get(GraphSONTokens.KEY), propData.get(GraphSONTokens.VALUE)); + }

          + }
          +
          + static class VertexPropertyJacksonDeserializer extends AbstractObjectDeserializer<VertexProperty> {
          +
          + protected VertexPropertyJacksonDeserializer()

          { + super(VertexProperty.class); + }

          +
          + @Override
          + public VertexProperty createObject(final Map<String, Object> propData) {
          + return propData.containsKey(GraphSONTokens.VERTEX) ?
          + new DetachedVertexProperty<>(
          + propData.get(GraphSONTokens.ID),
          + (String) propData.get(GraphSONTokens.LABEL),
          + propData.get(GraphSONTokens.VALUE), (Map<String, Object>) propData.get(GraphSONTokens.PROPERTIES),
          + new DetachedVertex(propData.get(GraphSONTokens.VERTEX), Vertex.DEFAULT_LABEL, null)) :
          — End diff –

          Supposing it's a Vertex with VertexProperties that is deserialized (and not only a VertexProperty on its own), will this really make the VertexProperty link to the right parent Vertex? It looks like this will create another one. However, I've recently found a nice way to make a deserializer sort of "context aware" in order to, when a Vertex deserializer deserializes its own VertexProperties, it can give to the VertexProperty deserializers the Vertex object that's "above" them - the VertexProperty's parent. I haven't had time to propose a fix here in TP but I intended to do so soon if you guys are interested

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88770063 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -459,9 +486,33 @@ public PropertyJacksonDeserializer() { @Override public Property createObject(final Map<String, Object> propData) { - return new DetachedProperty( - (String) propData.get(GraphSONTokens.KEY), - propData.get(GraphSONTokens.VALUE)); + final Object element = propData.get(GraphSONTokens.ELEMENT); + return element instanceof Element ? // graphson-non-embedded is treated differently, but since this is a hard coded embedding... + new DetachedProperty<>((String) propData.get(GraphSONTokens.KEY), propData.get(GraphSONTokens.VALUE), (Element) element) : + new DetachedProperty<>((String) propData.get(GraphSONTokens.KEY), propData.get(GraphSONTokens.VALUE)); + } + } + + static class VertexPropertyJacksonDeserializer extends AbstractObjectDeserializer<VertexProperty> { + + protected VertexPropertyJacksonDeserializer() { + super(VertexProperty.class); + } + + @Override + public VertexProperty createObject(final Map<String, Object> propData) { + return propData.containsKey(GraphSONTokens.VERTEX) ? + new DetachedVertexProperty<>( + propData.get(GraphSONTokens.ID), + (String) propData.get(GraphSONTokens.LABEL), + propData.get(GraphSONTokens.VALUE), (Map<String, Object>) propData.get(GraphSONTokens.PROPERTIES), + new DetachedVertex(propData.get(GraphSONTokens.VERTEX), Vertex.DEFAULT_LABEL, null)) : — End diff – Supposing it's a Vertex with VertexProperties that is deserialized (and not only a VertexProperty on its own), will this really make the VertexProperty link to the right parent Vertex? It looks like this will create another one. However, I've recently found a nice way to make a deserializer sort of "context aware" in order to, when a Vertex deserializer deserializes its own VertexProperties, it can give to the VertexProperty deserializers the Vertex object that's "above" them - the VertexProperty's parent. I haven't had time to propose a fix here in TP but I intended to do so soon if you guys are interested
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          I sneaked in and made a few comments I hope that's ok

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/499 I sneaked in and made a few comments I hope that's ok
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88914949

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -175,8 +177,30 @@ public PropertyJacksonSerializer() {
          public void serialize(final Property property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider)
          throws IOException {
          jsonGenerator.writeStartObject();

          • jsonGenerator.writeObjectField(GraphSONTokens.KEY, property.key());
            + jsonGenerator.writeStringField(GraphSONTokens.KEY, property.key());
            jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value());
            + if (property.element() instanceof VertexProperty) { + VertexProperty vertexProperty = (VertexProperty) property.element(); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT); + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:VertexProperty"); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.VALUEPROP); + jsonGenerator.writeObjectField(GraphSONTokens.ID, vertexProperty.id()); + jsonGenerator.writeStringField(GraphSONTokens.LABEL, vertexProperty.label()); + jsonGenerator.writeObjectField(GraphSONTokens.VERTEX, vertexProperty.element().id()); + jsonGenerator.writeEndObject(); + jsonGenerator.writeEndObject(); + }

            else if (property.element() instanceof Edge) {
            + Edge edge = (Edge) property.element();
            + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT);
            + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:Edge");

              • End diff –

          You can't always expect `outVLabel` and `inVLabel` to exist. Think about `ReferenceEdge`. It would be too expensive to always include such information. The most important think about the serialization is "can it be attached?" The way GraphSON 2.0 was designed, this was not the case. How do you attach a property? an vertex property? Finally, given the attach requirements of an edge, you have the problem of:

          property -> edge -> vertex (which vertex – in or out?) ...

          The amount of data required to attach a property would be pretty nasty. Hence, the "reference" serialization is smaller.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88914949 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -175,8 +177,30 @@ public PropertyJacksonSerializer() { public void serialize(final Property property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider) throws IOException { jsonGenerator.writeStartObject(); jsonGenerator.writeObjectField(GraphSONTokens.KEY, property.key()); + jsonGenerator.writeStringField(GraphSONTokens.KEY, property.key()); jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value()); + if (property.element() instanceof VertexProperty) { + VertexProperty vertexProperty = (VertexProperty) property.element(); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT); + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:VertexProperty"); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.VALUEPROP); + jsonGenerator.writeObjectField(GraphSONTokens.ID, vertexProperty.id()); + jsonGenerator.writeStringField(GraphSONTokens.LABEL, vertexProperty.label()); + jsonGenerator.writeObjectField(GraphSONTokens.VERTEX, vertexProperty.element().id()); + jsonGenerator.writeEndObject(); + jsonGenerator.writeEndObject(); + } else if (property.element() instanceof Edge) { + Edge edge = (Edge) property.element(); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT); + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:Edge"); End diff – You can't always expect `outVLabel` and `inVLabel` to exist. Think about `ReferenceEdge`. It would be too expensive to always include such information. The most important think about the serialization is "can it be attached?" The way GraphSON 2.0 was designed, this was not the case. How do you attach a property? an vertex property? Finally, given the attach requirements of an edge, you have the problem of: property -> edge -> vertex (which vertex – in or out?) ... The amount of data required to attach a property would be pretty nasty. Hence, the "reference" serialization is smaller.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r88915459

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -175,8 +177,30 @@ public PropertyJacksonSerializer() {
          public void serialize(final Property property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider)
          throws IOException {
          jsonGenerator.writeStartObject();

          • jsonGenerator.writeObjectField(GraphSONTokens.KEY, property.key());
            + jsonGenerator.writeStringField(GraphSONTokens.KEY, property.key());
            jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value());
            + if (property.element() instanceof VertexProperty) {
            + VertexProperty vertexProperty = (VertexProperty) property.element();
            + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT);
            + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:VertexProperty");
            + jsonGenerator.writeObjectFieldStart(GraphSONTokens.VALUEPROP);
            + jsonGenerator.writeObjectField(GraphSONTokens.ID, vertexProperty.id());
            + jsonGenerator.writeStringField(GraphSONTokens.LABEL, vertexProperty.label());
            + jsonGenerator.writeObjectField(GraphSONTokens.VERTEX, vertexProperty.element().id());
              • End diff –

          Because a `VertexProperty` is attached to a `Vertex` and thus, generalized `Element` semantics would only cause a chain of JSON unnecessary when `Vertex` attachement only requires a single ID – the vertex id.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r88915459 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -175,8 +177,30 @@ public PropertyJacksonSerializer() { public void serialize(final Property property, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider) throws IOException { jsonGenerator.writeStartObject(); jsonGenerator.writeObjectField(GraphSONTokens.KEY, property.key()); + jsonGenerator.writeStringField(GraphSONTokens.KEY, property.key()); jsonGenerator.writeObjectField(GraphSONTokens.VALUE, property.value()); + if (property.element() instanceof VertexProperty) { + VertexProperty vertexProperty = (VertexProperty) property.element(); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.ELEMENT); + jsonGenerator.writeStringField(GraphSONTokens.VALUETYPE, "g:VertexProperty"); + jsonGenerator.writeObjectFieldStart(GraphSONTokens.VALUEPROP); + jsonGenerator.writeObjectField(GraphSONTokens.ID, vertexProperty.id()); + jsonGenerator.writeStringField(GraphSONTokens.LABEL, vertexProperty.label()); + jsonGenerator.writeObjectField(GraphSONTokens.VERTEX, vertexProperty.element().id()); End diff – Because a `VertexProperty` is attached to a `Vertex` and thus, generalized `Element` semantics would only cause a chain of JSON unnecessary when `Vertex` attachement only requires a single ID – the vertex id.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89010143

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final
          }

          private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException {

          • final Iterator<Property<Object>> elementProperties = normalize ?
            + final Iterator<Property<Object>> edgeProperties = normalize ?
            IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties();
          • if (elementProperties.hasNext()) {
            + if (edgeProperties.hasNext()) {
            jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES);

          jsonGenerator.writeStartObject();

          • elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop));
            + while (edgeProperties.hasNext()) {
              • End diff –

          I had noticed the discrepancy in that VertexProperty's Properties are not serialized as Properties but instead the values directly are serialized and that's an oversight from when I implemented GraphSON2. I understand that the same thing is applied here and it makes a smaller footprint but I think we should rather fix the VertexProperty's properties to be serialized as Map of Properties and not Map of the values, because it reflects directly what is going on underneath and so the deserialization process does not need to any transformation, just set the fields of the newly created object with the rest of the deserialized data, rather than creating. Also it is not consistent because each Vertex, Edge, and VertexProperty derive from Element, which has the same `properties` field, a list of `? extends Property` so we should come up with the same format for all I believe and here a Vertex would have its properties serialized objects, but the others would have their properties serialized as the values, not sure if that's a major concern but I would advocate for consistency as much as possible

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89010143 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final } private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException { final Iterator<Property<Object>> elementProperties = normalize ? + final Iterator<Property<Object>> edgeProperties = normalize ? IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties(); if (elementProperties.hasNext()) { + if (edgeProperties.hasNext()) { jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES); jsonGenerator.writeStartObject(); elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop)); + while (edgeProperties.hasNext()) { End diff – I had noticed the discrepancy in that VertexProperty's Properties are not serialized as Properties but instead the values directly are serialized and that's an oversight from when I implemented GraphSON2. I understand that the same thing is applied here and it makes a smaller footprint but I think we should rather fix the VertexProperty's properties to be serialized as Map of Properties and not Map of the values, because it reflects directly what is going on underneath and so the deserialization process does not need to any transformation, just set the fields of the newly created object with the rest of the deserialized data, rather than creating. Also it is not consistent because each Vertex, Edge, and VertexProperty derive from Element, which has the same `properties` field, a list of `? extends Property` so we should come up with the same format for all I believe and here a Vertex would have its properties serialized objects, but the others would have their properties serialized as the values, not sure if that's a major concern but I would advocate for consistency as much as possible
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89010235

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final
          }

          private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException {

          • final Iterator<Property<Object>> elementProperties = normalize ?
            + final Iterator<Property<Object>> edgeProperties = normalize ?
            IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties();
          • if (elementProperties.hasNext()) {
            + if (edgeProperties.hasNext()) {
            jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES);

          jsonGenerator.writeStartObject();

          • elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop));
            + while (edgeProperties.hasNext()) {
            + final Property<?> property = edgeProperties.next();
            + jsonGenerator.writeObjectField(property.key(), property.value());
              • End diff –

          I had noticed the discrepancy in that VertexProperty's Properties are not serialized as Properties but instead the values directly are serialized and that's an oversight from when I implemented GraphSON2. I understand that the same thing is applied here and it makes a smaller footprint but I think we should rather fix the VertexProperty's properties to be serialized as Map of Properties and not Map of the values, because it reflects directly what is going on underneath and so the deserialization process does not need to any transformation, just set the fields of the newly created object with the rest of the deserialized data, rather than creating. Also it is not consistent because each Vertex, Edge, and VertexProperty derive from Element, which has the same properties field, a list of ? extends Property so we should come up with the same format for all I believe and here a Vertex would have its properties serialized objects, but the others would have their properties serialized as the values, not sure if that's a major concern but I would advocate for consistency as much as possible

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89010235 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final } private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException { final Iterator<Property<Object>> elementProperties = normalize ? + final Iterator<Property<Object>> edgeProperties = normalize ? IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties(); if (elementProperties.hasNext()) { + if (edgeProperties.hasNext()) { jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES); jsonGenerator.writeStartObject(); elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop)); + while (edgeProperties.hasNext()) { + final Property<?> property = edgeProperties.next(); + jsonGenerator.writeObjectField(property.key(), property.value()); End diff – I had noticed the discrepancy in that VertexProperty's Properties are not serialized as Properties but instead the values directly are serialized and that's an oversight from when I implemented GraphSON2. I understand that the same thing is applied here and it makes a smaller footprint but I think we should rather fix the VertexProperty's properties to be serialized as Map of Properties and not Map of the values, because it reflects directly what is going on underneath and so the deserialization process does not need to any transformation, just set the fields of the newly created object with the rest of the deserialized data, rather than creating. Also it is not consistent because each Vertex, Edge, and VertexProperty derive from Element, which has the same properties field, a list of ? extends Property so we should come up with the same format for all I believe and here a Vertex would have its properties serialized objects, but the others would have their properties serialized as the values, not sure if that's a major concern but I would advocate for consistency as much as possible
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89010296

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -459,9 +486,33 @@ public PropertyJacksonDeserializer() {

          @Override
          public Property createObject(final Map<String, Object> propData)

          { - return new DetachedProperty( - (String) propData.get(GraphSONTokens.KEY), - propData.get(GraphSONTokens.VALUE)); + final Object element = propData.get(GraphSONTokens.ELEMENT); + return element instanceof Element ? // graphson-non-embedded is treated differently, but since this is a hard coded embedding... + new DetachedProperty<>((String) propData.get(GraphSONTokens.KEY), propData.get(GraphSONTokens.VALUE), (Element) element) : + new DetachedProperty<>((String) propData.get(GraphSONTokens.KEY), propData.get(GraphSONTokens.VALUE)); + }

          + }
          +
          + static class VertexPropertyJacksonDeserializer extends AbstractObjectDeserializer<VertexProperty> {
          +
          + protected VertexPropertyJacksonDeserializer()

          { + super(VertexProperty.class); + }

          +
          + @Override
          + public VertexProperty createObject(final Map<String, Object> propData) {
          + return propData.containsKey(GraphSONTokens.VERTEX) ?
          + new DetachedVertexProperty<>(
          + propData.get(GraphSONTokens.ID),
          + (String) propData.get(GraphSONTokens.LABEL),
          + propData.get(GraphSONTokens.VALUE), (Map<String, Object>) propData.get(GraphSONTokens.PROPERTIES),
          + new DetachedVertex(propData.get(GraphSONTokens.VERTEX), Vertex.DEFAULT_LABEL, null)) :
          — End diff –

          I don't quite follow what you are saying, but if you have a cool idea with "context awareness," by all means, push a patch to this branch.

          Here is the thing about being able to attach an element back to the main graph.

          • `Vertex`: All you need is the vertex ID.
          • `Edge`: Edge ID (but for most dbs, outV id and edge label too)
          • `VertexProperty`: vertex property ID, vertex ID (but for optimization in most dbs, label too).
          • `Property`: key/value, element id. Given that properties are ONLY on vertex properties and edges, then you need the requisite information about the element for attachment.
          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89010296 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -459,9 +486,33 @@ public PropertyJacksonDeserializer() { @Override public Property createObject(final Map<String, Object> propData) { - return new DetachedProperty( - (String) propData.get(GraphSONTokens.KEY), - propData.get(GraphSONTokens.VALUE)); + final Object element = propData.get(GraphSONTokens.ELEMENT); + return element instanceof Element ? // graphson-non-embedded is treated differently, but since this is a hard coded embedding... + new DetachedProperty<>((String) propData.get(GraphSONTokens.KEY), propData.get(GraphSONTokens.VALUE), (Element) element) : + new DetachedProperty<>((String) propData.get(GraphSONTokens.KEY), propData.get(GraphSONTokens.VALUE)); + } + } + + static class VertexPropertyJacksonDeserializer extends AbstractObjectDeserializer<VertexProperty> { + + protected VertexPropertyJacksonDeserializer() { + super(VertexProperty.class); + } + + @Override + public VertexProperty createObject(final Map<String, Object> propData) { + return propData.containsKey(GraphSONTokens.VERTEX) ? + new DetachedVertexProperty<>( + propData.get(GraphSONTokens.ID), + (String) propData.get(GraphSONTokens.LABEL), + propData.get(GraphSONTokens.VALUE), (Map<String, Object>) propData.get(GraphSONTokens.PROPERTIES), + new DetachedVertex(propData.get(GraphSONTokens.VERTEX), Vertex.DEFAULT_LABEL, null)) : — End diff – I don't quite follow what you are saying, but if you have a cool idea with "context awareness," by all means, push a patch to this branch. Here is the thing about being able to attach an element back to the main graph. `Vertex`: All you need is the vertex ID. `Edge`: Edge ID (but for most dbs, outV id and edge label too) `VertexProperty`: vertex property ID, vertex ID (but for optimization in most dbs, label too). `Property`: key/value, element id. Given that properties are ONLY on vertex properties and edges, then you need the requisite information about the element for attachment.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89010633

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final
          }

          private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException {

          • final Iterator<Property<Object>> elementProperties = normalize ?
            + final Iterator<Property<Object>> edgeProperties = normalize ?
            IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties();
          • if (elementProperties.hasNext()) {
            + if (edgeProperties.hasNext()) {
            jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES);

          jsonGenerator.writeStartObject();

          • elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop));
            + while (edgeProperties.hasNext()) {
            + final Property<?> property = edgeProperties.next();
            + jsonGenerator.writeObjectField(property.key(), property.value());
              • End diff –

          Why don't you propose a GraphSON 2 format for Vertex, Edge, Property, VertexProperty in a JSON document that we can look at. From there, without how to implenent the serializers/deserailizers, we can see your arguments separate from code. Then if they are good, we take the respective code patch. Thoughts?

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89010633 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final } private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException { final Iterator<Property<Object>> elementProperties = normalize ? + final Iterator<Property<Object>> edgeProperties = normalize ? IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties(); if (elementProperties.hasNext()) { + if (edgeProperties.hasNext()) { jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES); jsonGenerator.writeStartObject(); elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop)); + while (edgeProperties.hasNext()) { + final Property<?> property = edgeProperties.next(); + jsonGenerator.writeObjectField(property.key(), property.value()); End diff – Why don't you propose a GraphSON 2 format for Vertex, Edge, Property, VertexProperty in a JSON document that we can look at. From there, without how to implenent the serializers/deserailizers, we can see your arguments separate from code. Then if they are good, we take the respective code patch. Thoughts?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89010812

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final
          }

          private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException {

          • final Iterator<Property<Object>> elementProperties = normalize ?
            + final Iterator<Property<Object>> edgeProperties = normalize ?
            IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties();
          • if (elementProperties.hasNext()) {
            + if (edgeProperties.hasNext()) {
            jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES);

          jsonGenerator.writeStartObject();

          • elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop));
            + while (edgeProperties.hasNext()) {
            + final Property<?> property = edgeProperties.next();
            + jsonGenerator.writeObjectField(property.key(), property.value());
              • End diff –

          Also I had waited before correcting the discrepancy mentioned above for the VertexProperty's properties because it's a breaking change and it seems dangerous to make it part of GraphSON 2.*0* in my opinion

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89010812 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final } private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException { final Iterator<Property<Object>> elementProperties = normalize ? + final Iterator<Property<Object>> edgeProperties = normalize ? IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties(); if (elementProperties.hasNext()) { + if (edgeProperties.hasNext()) { jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES); jsonGenerator.writeStartObject(); elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop)); + while (edgeProperties.hasNext()) { + final Property<?> property = edgeProperties.next(); + jsonGenerator.writeObjectField(property.key(), property.value()); End diff – Also I had waited before correcting the discrepancy mentioned above for the VertexProperty's properties because it's a breaking change and it seems dangerous to make it part of GraphSON 2.* 0 * in my opinion
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89014281

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final
          }

          private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException {

          • final Iterator<Property<Object>> elementProperties = normalize ?
            + final Iterator<Property<Object>> edgeProperties = normalize ?
            IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties();
          • if (elementProperties.hasNext()) {
            + if (edgeProperties.hasNext()) {
            jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES);

          jsonGenerator.writeStartObject();

          • elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop));
            + while (edgeProperties.hasNext()) {
            + final Property<?> property = edgeProperties.next();
            + jsonGenerator.writeObjectField(property.key(), property.value());
              • End diff –

          Basically I had written [this for some specs](https://gist.github.com/newkek/15e9933dc4b43674c2c60bc9487fc80e#file-graph-types-asciidoc) (note: not sure the Metrics and TraversalMetrics ones are still up to date but the rest should be).
          What I suggest is [the revision here](https://gist.github.com/newkek/15e9933dc4b43674c2c60bc9487fc80e/revisions#diff-5a08d02157ba6ec13bacc40ab3a3a7bc).

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89014281 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final } private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException { final Iterator<Property<Object>> elementProperties = normalize ? + final Iterator<Property<Object>> edgeProperties = normalize ? IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties(); if (elementProperties.hasNext()) { + if (edgeProperties.hasNext()) { jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES); jsonGenerator.writeStartObject(); elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop)); + while (edgeProperties.hasNext()) { + final Property<?> property = edgeProperties.next(); + jsonGenerator.writeObjectField(property.key(), property.value()); End diff – Basically I had written [this for some specs] ( https://gist.github.com/newkek/15e9933dc4b43674c2c60bc9487fc80e#file-graph-types-asciidoc ) (note: not sure the Metrics and TraversalMetrics ones are still up to date but the rest should be). What I suggest is [the revision here] ( https://gist.github.com/newkek/15e9933dc4b43674c2c60bc9487fc80e/revisions#diff-5a08d02157ba6ec13bacc40ab3a3a7bc ).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89014561

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java —
          @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final
          }

          private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException {

          • final Iterator<Property<Object>> elementProperties = normalize ?
            + final Iterator<Property<Object>> edgeProperties = normalize ?
            IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties();
          • if (elementProperties.hasNext()) {
            + if (edgeProperties.hasNext()) {
            jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES);

          jsonGenerator.writeStartObject();

          • elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop));
            + while (edgeProperties.hasNext()) {
            + final Property<?> property = edgeProperties.next();
            + jsonGenerator.writeObjectField(property.key(), property.value());
              • End diff –

          What you suggest in the PR here is [this](https://gist.github.com/newkek/15e9933dc4b43674c2c60bc9487fc80e/revisions#diff-5a08d02157ba6ec13bacc40ab3a3a7bc)

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89014561 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONSerializersV2d0.java — @@ -153,13 +152,16 @@ public void serialize(final Edge edge, final JsonGenerator jsonGenerator, final } private void writeProperties(final Edge edge, final JsonGenerator jsonGenerator) throws IOException { final Iterator<Property<Object>> elementProperties = normalize ? + final Iterator<Property<Object>> edgeProperties = normalize ? IteratorUtils.list(edge.properties(), Comparators.PROPERTY_COMPARATOR).iterator() : edge.properties(); if (elementProperties.hasNext()) { + if (edgeProperties.hasNext()) { jsonGenerator.writeFieldName(GraphSONTokens.PROPERTIES); jsonGenerator.writeStartObject(); elementProperties.forEachRemaining(prop -> safeWriteObjectField(jsonGenerator, prop.key(), prop)); + while (edgeProperties.hasNext()) { + final Property<?> property = edgeProperties.next(); + jsonGenerator.writeObjectField(property.key(), property.value()); End diff – What you suggest in the PR here is [this] ( https://gist.github.com/newkek/15e9933dc4b43674c2c60bc9487fc80e/revisions#diff-5a08d02157ba6ec13bacc40ab3a3a7bc )
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          Please put it in a form like: https://github.com/apache/tinkerpop/blob/e09309c1210ad5d7d5f33440d4d8d1470bd3f0e2/docs/src/dev/io/graphson.asciidoc .. What you mean by "data" doesn't make sense to me. If we see:

          ```json
          { "@type" : "g:Blah",
          "@value":

          { "x" : "y", "a" : "b" }

          }
          ```

          ...it will be more clear what you are proposing.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 Please put it in a form like: https://github.com/apache/tinkerpop/blob/e09309c1210ad5d7d5f33440d4d8d1470bd3f0e2/docs/src/dev/io/graphson.asciidoc .. What you mean by "data" doesn't make sense to me. If we see: ```json { "@type" : "g:Blah", "@value": { "x" : "y", "a" : "b" } } ``` ...it will be more clear what you are proposing.
          Hide
          githubbot ASF GitHub Bot added a comment -
          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/499 https://gist.github.com/newkek/bfc976712f1adcbe76bcea4e34435ea2 - the rest doesn't change
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          If we do this, then we would have to change `Edge` back to the same model to be consistent. However, why change it back? We gain the benefit of less verbosity.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 If we do this, then we would have to change `Edge` back to the same model to be consistent. However, why change it back? We gain the benefit of less verbosity.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          > If we do this, then we would have to change Edge back to the same model to be consistent

          Indeed.

          > why change it back?

          Because currently it is not consistent, (Detached)Edge/Vertex/VertexProperty all derive from (Detached)Element, for which `properties` have the same structure, but with what's currently on the PR, Edge and VertexProperty don't have their `properties` serialized in the same format than Vertex.
          If we unify all we can make generalize the deserialization of an object that derives from Element and make the deserialization code more readable.

          In my opinion I don't believe verbosity should be chosen over consistency here, especially since GraphSON 2 is already very very verbose and not really meant to be compact.

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/499 > If we do this, then we would have to change Edge back to the same model to be consistent Indeed. > why change it back? Because currently it is not consistent, (Detached)Edge/Vertex/VertexProperty all derive from (Detached)Element, for which `properties` have the same structure, but with what's currently on the PR, Edge and VertexProperty don't have their `properties` serialized in the same format than Vertex. If we unify all we can make generalize the deserialization of an object that derives from Element and make the deserialization code more readable. In my opinion I don't believe verbosity should be chosen over consistency here, especially since GraphSON 2 is already very very verbose and not really meant to be compact.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          Talking with @spmallette. There are lots of issues with GraphSON 2.0 that both you and I notice. Lets do this. Lets get this ticket merged as there are lots of other work in here and start up a new ticket for proposed GraphSON 2.0 changes that will target the same branch as the merge. I can create a new JIRA and will add the link.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 Talking with @spmallette. There are lots of issues with GraphSON 2.0 that both you and I notice. Lets do this. Lets get this ticket merged as there are lots of other work in here and start up a new ticket for proposed GraphSON 2.0 changes that will target the same branch as the merge. I can create a new JIRA and will add the link.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          @okram what aspect of this PR should we be considering then for purposes of review? do you have a brief synopsis of that and what is on the table for change later?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/499 @okram what aspect of this PR should we be considering then for purposes of review? do you have a brief synopsis of that and what is on the table for change later?
          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 https://issues.apache.org/jira/browse/TINKERPOP-1565
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          @spmallette — we simply can't abandon the GraphSON 2.0 changes I made cause we need to be able to "attach properties" (vertex properties and element properties). However, there are decisions to be made on how to do that (as outlined by @newkek). Moreover, there are more general decisions to be made about GraphSON 2.0 (https://issues.apache.org/jira/browse/TINKERPOP-1565). I think we should simply review this for what it is (ignore discussions of "best GraphSON 2.0" representation and be sure that we do https://issues.apache.org/jira/browse/TINKERPOP-1565 to come to a consensus on GraphSON 2.0 for the next release.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 @spmallette — we simply can't abandon the GraphSON 2.0 changes I made cause we need to be able to "attach properties" (vertex properties and element properties). However, there are decisions to be made on how to do that (as outlined by @newkek). Moreover, there are more general decisions to be made about GraphSON 2.0 ( https://issues.apache.org/jira/browse/TINKERPOP-1565 ). I think we should simply review this for what it is (ignore discussions of "best GraphSON 2.0" representation and be sure that we do https://issues.apache.org/jira/browse/TINKERPOP-1565 to come to a consensus on GraphSON 2.0 for the next release.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          Another thing we can do that I think would be best is target this to `master/`. Why?

          1. There are changes to Gryo we will need to make so ensure "attach-ability."
          2. This gives us more leeway around breaking GraphSON 2.0 changes.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 Another thing we can do that I think would be best is target this to `master/`. Why? 1. There are changes to Gryo we will need to make so ensure "attach-ability." 2. This gives us more leeway around breaking GraphSON 2.0 changes.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          If we do that will there be "bad" bugs left in tp32 that won't get fixed?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/499 If we do that will there be "bad" bugs left in tp32 that won't get fixed?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          This ticket doesn't address a "bug" as much as a consistency issue. The primary issue is that Bytecode and GraphTraversal are not in one-to-one correspondence. You get the same "answer," but for example: `hasId(1)` is represented in bytecode as `has(T.id,1)`. We can just kick this consistency verification to 3.3.0.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 This ticket doesn't address a "bug" as much as a consistency issue. The primary issue is that Bytecode and GraphTraversal are not in one-to-one correspondence. You get the same "answer," but for example: `hasId(1)` is represented in bytecode as `has(T.id,1)`. We can just kick this consistency verification to 3.3.0.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user newkek commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          Thanks for the separate ticket, I'll put comments over there. So All the following items (from the PR desc):

          > - There was a GraphSON 2.0 issue with Edge properties. Changed to the same format a VertexProperties. Smaller footprint and only necessary information.

          • Added VertexID ("vertex"-field) to VertexProperty in GraphSON 2.0 – how else will you attach?
          • Added Element ("element"-field) to Property in GraphSON 2.0 – how else will you attach?

          Will be handled separately in TINKERPOP-1565, and GraphSON2 will not change in this PR, right? (just trying to understand because it has external impact to change GraphSON2)

          Show
          githubbot ASF GitHub Bot added a comment - Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/499 Thanks for the separate ticket, I'll put comments over there. So All the following items (from the PR desc): > - There was a GraphSON 2.0 issue with Edge properties. Changed to the same format a VertexProperties. Smaller footprint and only necessary information. Added VertexID ("vertex"-field) to VertexProperty in GraphSON 2.0 – how else will you attach? Added Element ("element"-field) to Property in GraphSON 2.0 – how else will you attach? Will be handled separately in TINKERPOP-1565 , and GraphSON2 will not change in this PR, right? (just trying to understand because it has external impact to change GraphSON2)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          No GraphSON will change in this branch but we will solve TINKERPOP-1562 in the same release as this branch. Thus, this branch is like a "half-ticket" with TINKERPOP-1562's completion being the finale. Note that I also targeted this branch to `master/` so we can have breaking changes to GraphSON 2.0 for TinkerPop 3.3.0.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 No GraphSON will change in this branch but we will solve TINKERPOP-1562 in the same release as this branch. Thus, this branch is like a "half-ticket" with TINKERPOP-1562 's completion being the finale. Note that I also targeted this branch to `master/` so we can have breaking changes to GraphSON 2.0 for TinkerPop 3.3.0.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          Integration tests ran against `master/` (the new target branch).

          ```
          [INFO] ------------------------------------------------------------------------
          [INFO] BUILD SUCCESS
          [INFO] ------------------------------------------------------------------------
          [INFO] Total time: 04:14 h
          [INFO] Finished at: 2016-11-22T23:02:41-07:00
          [INFO] Final Memory: 179M/1656M
          [INFO] ------------------------------------------------------------------------
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 Integration tests ran against `master/` (the new target branch). ``` [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 04:14 h [INFO] Finished at: 2016-11-22T23:02:41-07:00 [INFO] Final Memory: 179M/1656M [INFO] ------------------------------------------------------------------------ ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          `docker/build.sh -t -i -n` succeeded.

          VOTE: +1

          (don't blame me for voting on half-done work; @okram assured me that we won't have a release before the work, started in this PR, is actually done)

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/499 `docker/build.sh -t -i -n` succeeded. VOTE: +1 (don't blame me for voting on half-done work; @okram assured me that we won't have a release before the work, started in this PR, is actually done)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          You are a bad person @dkuppitz.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/499 You are a bad person @dkuppitz.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89348850

          — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java —
          @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() {
          }
          }
          }
          +
          + @Test
          + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception {
          + final Random random = new Random();
          — End diff –

          did you see my comment above @okram ? can we go with option 1 for this PR?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89348850 — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java — @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() { } } } + + @Test + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception { + final Random random = new Random(); — End diff – did you see my comment above @okram ? can we go with option 1 for this PR?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89352742

          — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java —
          @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() {
          }
          }
          }
          +
          + @Test
          + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception {
          + final Random random = new Random();
          — End diff –

          Ah yea. Thats smart. Do you want to push that to this branch or tell me how to do it with an example?

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89352742 — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java — @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() { } } } + + @Test + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception { + final Random random = new Random(); — End diff – Ah yea. Thats smart. Do you want to push that to this branch or tell me how to do it with an example?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/499#discussion_r89358284

          — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java —
          @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() {
          }
          }
          }
          +
          + @Test
          + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception {
          + final Random random = new Random();
          — End diff –

          I can amend the branch

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/499#discussion_r89358284 — Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java — @@ -71,4 +80,89 @@ public void graphTraversalShouldHaveMethodsOfAnonymousGraphTraversal() { } } } + + @Test + public void graphTraversalMethodsShouldGenerateCorrespondingBytecode() throws Exception { + final Random random = new Random(); — End diff – I can amend the branch
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/499

          I added the logging of the seed for that test that uses `Random`: 73859cce8563c3951e39fef58b189acb9718e5c3

          It's made for one time use right now - we probably could generalize that a bit for re-use in other tests that use `Random` (and it doesn't make sense to wholly factor out the `Random`).

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/499 I added the logging of the seed for that test that uses `Random`: 73859cce8563c3951e39fef58b189acb9718e5c3 It's made for one time use right now - we probably could generalize that a bit for re-use in other tests that use `Random` (and it doesn't make sense to wholly factor out the `Random`). VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/499

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/499

            People

            • Assignee:
              okram Marko A. Rodriguez
              Reporter:
              andrew.tolbert Andy Tolbert
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development