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

Optional/Coalesce should not allow sideEffect traversals.

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.1.4, 3.2.2
    • Fix Version/s: 3.3.0
    • Component/s: process
    • Labels:
      None

      Description

      It took me a long time to realize what was wrong with a traversal I wrote that used optional(blah.sideEffect.blah). optional() maps to ChooseStep under the hood and the provide traversal is first tested for a hasNext(). If so, the it plays itself out. The problem is that if there is a side-effect in the traversal child, then it gets executed twice.

      gremlin> g = TinkerGraph.open().traversal()
      ==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
      gremlin> g.inject(1).optional(addV('twin'))
      ==>v[1]
      gremlin> g.V().valueMap(true)
      ==>[id:0,label:twin]
      ==>[id:1,label:twin]
      

      We should NOT allow optional() to have SideEffectStep steps in it so as not to cause unexpected behavior. StandardVerificationStrategy can analyze and throw an exception if necessary.

      Also, coalesce() has a similar problem, though perhaps it can be a useful 'technique.'

      gremlin> g = TinkerGraph.open().traversal()
      ==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
      gremlin> g.inject(1).coalesce(addV('twin1').limit(0), addV('twin2'))
      ==>v[1]
      gremlin> g.V().valueMap(true)
      ==>[id:0,label:twin1]
      ==>[id:1,label:twin2]
      gremlin>
      

        Issue Links

          Activity

          Hide
          rdale Robert Dale added a comment -

          I don't use optional() but I wonder if that's more of a bug. Why execute twice? Shouldn't next() just return the result of hasNext()? It almost sounds like this bug https://github.com/apache/tinkerpop/pull/346

          Also, I disagree on coalesce(). It should continue to work as-is. It's very useful for upsert (update or insert) where you definitely want the second traversal to create an element. I'm open to a better way. But in general, just because you can shoot yourself in the foot, it doesn't make the tool defective. Maybe it would be better to put a NOTE: in the ref guides about doing such things to make to explicitly clear.

          Show
          rdale Robert Dale added a comment - I don't use optional() but I wonder if that's more of a bug. Why execute twice? Shouldn't next() just return the result of hasNext()? It almost sounds like this bug https://github.com/apache/tinkerpop/pull/346 Also, I disagree on coalesce(). It should continue to work as-is. It's very useful for upsert (update or insert) where you definitely want the second traversal to create an element. I'm open to a better way. But in general, just because you can shoot yourself in the foot, it doesn't make the tool defective. Maybe it would be better to put a NOTE: in the ref guides about doing such things to make to explicitly clear.
          Hide
          okram Marko A. Rodriguez added a comment -

          So, we could make it so that next() returns the result from hasNext(). However, the way BranchStep is implemented (for which ChooseStep is a subclass (for which optional() is implemented with)), the selection traversal is not the same as the branch traversal. Where the former is a "local child" and the latter, a "global child." Thus, optional() uses clone() to get a selection and branch traversal. We could always write an OptionalStep which would do as you recommend and in fact, this might be the best way forward.

          Show
          okram Marko A. Rodriguez added a comment - So, we could make it so that next() returns the result from hasNext() . However, the way BranchStep is implemented (for which ChooseStep is a subclass (for which optional() is implemented with)), the selection traversal is not the same as the branch traversal. Where the former is a "local child" and the latter, a "global child." Thus, optional() uses clone() to get a selection and branch traversal. We could always write an OptionalStep which would do as you recommend and in fact, this might be the best way forward.
          Hide
          okram Marko A. Rodriguez added a comment -

          Huh. I just reailzed we can implement optional() using ColesceStep and we don't have this problem.

          gremlin> g.inject(1).coalesce(addV('twin'),identity())
          ==>v[0]
          gremlin> g.V()
          ==>v[0]
          

          Thus, optional -> coalesce(x,identity()). Easy fix. Any objections to this direction?

          Show
          okram Marko A. Rodriguez added a comment - Huh. I just reailzed we can implement optional() using ColesceStep and we don't have this problem. gremlin> g.inject(1).coalesce(addV('twin'),identity()) ==>v[0] gremlin> g.V() ==>v[0] Thus, optional -> coalesce(x,identity()) . Easy fix. Any objections to this direction?
          Hide
          pietermartin pieter martin added a comment -

          `coalesce` was discussed way back when `optional` was first discussed.
          Daniel Kuppitz comment seems to show that `coalesce` was not what we wanted.
          Marko A. Rodriguez comment indicated "Ah smart. The reason choose works and coalesce doesn't is because one uses globalTraversals and the other uses localTraversals"

          Do there comments still hold?

          Show
          pietermartin pieter martin added a comment - `coalesce` was discussed way back when `optional` was first discussed. Daniel Kuppitz comment seems to show that `coalesce` was not what we wanted. Marko A. Rodriguez comment indicated "Ah smart. The reason choose works and coalesce doesn't is because one uses globalTraversals and the other uses localTraversals" Do there comments still hold?
          Hide
          dkuppitz Daniel Kuppitz added a comment -

          These comments still hold true. optional and coalesce are not interchangeable.

          gremlin> g.V().coalesce(out("knows"), identity()).coalesce(out("created"), identity()).path()
          ==>[v[1],v[2],v[2]]
          ==>[v[1],v[4],v[5]]
          ==>[v[1],v[4],v[3]]
          ==>[v[2],v[2],v[2]]
          ==>[v[3],v[3],v[3]]
          ==>[v[4],v[4],v[5]]
          ==>[v[4],v[4],v[3]]
          ==>[v[5],v[5],v[5]]
          ==>[v[6],v[6],v[3]]
          gremlin> g.V().optional(out("knows")).optional(out("created")).path()
          ==>[v[1],v[2]]
          ==>[v[1],v[4],v[5]]
          ==>[v[1],v[4],v[3]]
          ==>[v[2]]
          ==>[v[3]]
          ==>[v[4],v[5]]
          ==>[v[4],v[3]]
          ==>[v[5]]
          ==>[v[6],v[3]]
          
          Show
          dkuppitz Daniel Kuppitz added a comment - These comments still hold true. optional and coalesce are not interchangeable. gremlin> g.V().coalesce(out("knows"), identity()).coalesce(out("created"), identity()).path() ==>[v[1],v[2],v[2]] ==>[v[1],v[4],v[5]] ==>[v[1],v[4],v[3]] ==>[v[2],v[2],v[2]] ==>[v[3],v[3],v[3]] ==>[v[4],v[4],v[5]] ==>[v[4],v[4],v[3]] ==>[v[5],v[5],v[5]] ==>[v[6],v[6],v[3]] gremlin> g.V().optional(out("knows")).optional(out("created")).path() ==>[v[1],v[2]] ==>[v[1],v[4],v[5]] ==>[v[1],v[4],v[3]] ==>[v[2]] ==>[v[3]] ==>[v[4],v[5]] ==>[v[4],v[3]] ==>[v[5]] ==>[v[6],v[3]]
          Hide
          dkuppitz Daniel Kuppitz added a comment -

          Btw., I don't think we should disallow nested side-effect steps, instead we should make it right. The double execution of those nested traversals is a well hidden performance killer - neither explain() nor profile() can reveal that there's something that is executed twice.

          Show
          dkuppitz Daniel Kuppitz added a comment - Btw., I don't think we should disallow nested side-effect steps, instead we should make it right. The double execution of those nested traversals is a well hidden performance killer - neither explain() nor profile() can reveal that there's something that is executed twice.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user okram opened a pull request:

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

          TINKERPOP-1506: Optional/Coalesce should not allow sideEffect traversals.

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

          `g.V(1).optional(addV())` created 2 vertices, not one. This is because `optional()` was implemented with the branch-step `ChooseStep`. This has been fixed with a custom `OptionalStep`. Test cases added to verify correct behavior.

          Added to `master/` as this is a breaking change. Though, the previous behavior can easily be deemed a bug.

          VOTE +1.

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

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

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

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


          commit 40971c95903077d38cb754843eb914b5548f5c1f
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-10-31T18:03:33Z

          added OptionalStep to be used instead of optional() being backed by ChooseStep. This fixes a side-effect issue.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user okram opened a pull request: https://github.com/apache/tinkerpop/pull/471 TINKERPOP-1506 : Optional/Coalesce should not allow sideEffect traversals. https://issues.apache.org/jira/browse/TINKERPOP-1506 `g.V(1).optional(addV())` created 2 vertices, not one. This is because `optional()` was implemented with the branch-step `ChooseStep`. This has been fixed with a custom `OptionalStep`. Test cases added to verify correct behavior. Added to `master/` as this is a breaking change. Though, the previous behavior can easily be deemed a bug. VOTE +1. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1506 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/471.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 #471 commit 40971c95903077d38cb754843eb914b5548f5c1f Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-10-31T18:03:33Z added OptionalStep to be used instead of optional() being backed by ChooseStep. This fixes a side-effect issue.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

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

          I've done some manual tests.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/471 I've done some manual tests. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twilmes commented on the issue:

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

          Nice find. Looks good.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user twilmes commented on the issue: https://github.com/apache/tinkerpop/pull/471 Nice find. Looks good. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user spmallette commented on the issue:

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

          All tests pass with `docker/build.sh -t -n -i`

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/471 All tests pass with `docker/build.sh -t -n -i` VOTE +1

            People

            • Assignee:
              okram Marko A. Rodriguez
              Reporter:
              okram Marko A. Rodriguez
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development