Details

    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 3.0.1-incubating
    • 3.1.0-incubating
    • process
    • None

    Description

      We should allow mid-traversal V() and E(). It shouldn't be a technical problem, since we always know the traversal source, thus it's just a simple flatMap. Vendors could then implement mid-traversal global index lookups.

      Example:

      g.V().hasLabel("company").has("name", "DataStax").in("worksFor").as("dse").
        V().hasLabel("company").has("name", "Aurelius").in("worksFor").as("ae").
            addOutE("dse", "knows", "ae", "since", "02/2015")
      

      Attachments

        Issue Links

          Activity

            So, does it work such that for each incoming object, V(). Or, we do a NullReducingBarrier and then V()?

            okram Marko A. Rodriguez added a comment - So, does it work such that for each incoming object, V() . Or, we do a NullReducingBarrier and then V() ?
            mhfrantz Matt Frantz added a comment -

            What about a structure that could be used as a source, and that would allow the intent to be more obvious, e.g. pairwise vs. cross-product, etc. using logic like match?

            It seems preferable to allow initiation of this from a source, so that the strategy can decide which order to execute the two or more source traversals.

            Here is a "join" reasoning traversal, creating edges based on property matching:

            g.match(
              __.V().hasLabel('company').as('c'),
              __.V().hasLabel('person').as('p'),
              __.as('c').values('name').as('cname'),
              __.as('p').values('companyName').as('pname'))
              .where('cname', eq('pname')
              .dedup('c', 'p')
              .select('c', 'p')
              .addOutE('p', 'worksFor', 'c')
            

            Here is a "cross-product" traversal, materializing the NxN space, except the diagonal:

            g.match(
              __.V().hasLabel('person').as('a'),
              __.V().hasLabel('person').as('b'))
              .where('a', neq('b'))
              .select('a', 'b').as('pair')
              .addOutE('a', 'relatesTo', 'b')
              .select('pair').addOutE('b', 'relatesTo', 'a')
            
            mhfrantz Matt Frantz added a comment - What about a structure that could be used as a source, and that would allow the intent to be more obvious, e.g. pairwise vs. cross-product, etc. using logic like match ? It seems preferable to allow initiation of this from a source, so that the strategy can decide which order to execute the two or more source traversals. Here is a "join" reasoning traversal, creating edges based on property matching: g.match( __.V().hasLabel('company').as('c'), __.V().hasLabel('person').as('p'), __.as('c').values('name').as('cname'), __.as('p').values('companyName').as('pname')) .where('cname', eq('pname') .dedup('c', 'p') .select('c', 'p') .addOutE('p', 'worksFor', 'c') Here is a "cross-product" traversal, materializing the NxN space, except the diagonal: g.match( __.V().hasLabel('person').as('a'), __.V().hasLabel('person').as('b')) .where('a', neq('b')) .select('a', 'b').as('pair') .addOutE('a', 'relatesTo', 'b') .select('pair').addOutE('b', 'relatesTo', 'a')
            dkuppitz Daniel Kuppitz added a comment -

            For each incoming object; it's just a simple flatMap.

            mhfrantz: You won't need those complicated match patterns:

            Query 1:

            g.V().hasLabel('company').as('c','cv').
              V().hasLabel('person').as('p','pv').dedup('c','p').
              select('c','p').by('cname').by('pname').where('c', eq('p')).
              addOutE('pv', 'worksFor', 'cv')
            

            Query 2:

            g.V().hasLabel('person').as('a').
              V().hasLabel('person').as('b').
              where('a', neq('b')).
              addOutE('a', 'relatesTo', 'b').
              addOutE('b', 'relatesTo', 'a')
            
            dkuppitz Daniel Kuppitz added a comment - For each incoming object; it's just a simple flatMap. mhfrantz : You won't need those complicated match patterns: Query 1: g.V().hasLabel( 'company' ).as( 'c' , 'cv' ). V().hasLabel( 'person' ).as( 'p' , 'pv' ).dedup( 'c' , 'p' ). select( 'c' , 'p' ).by( 'cname' ).by( 'pname' ).where( 'c' , eq( 'p' )). addOutE( 'pv' , 'worksFor' , 'cv' ) Query 2: g.V().hasLabel( 'person' ).as( 'a' ). V().hasLabel( 'person' ).as( 'b' ). where( 'a' , neq( 'b' )). addOutE( 'a' , 'relatesTo' , 'b' ). addOutE( 'b' , 'relatesTo' , 'a' )
            mhfrantz Matt Frantz added a comment -

            I was thinking that the match step logic would allow the merge of multiple sources to be dynamically optimized, and would properly capture the symmetry of a multi-source traversal. If written as a mid-traversal step, the author must decide which traversal is the "outer loop" and which the "inner loop", or else we rely on a strategy that splits and rewrites the traversal using the match step logic.

            So perhaps we can start with the imperative mid-traversal step (this JIRA issue), and then work toward allowing match to optimize multiple sources (a separate JIRA issue).

            mhfrantz Matt Frantz added a comment - I was thinking that the match step logic would allow the merge of multiple sources to be dynamically optimized, and would properly capture the symmetry of a multi-source traversal. If written as a mid-traversal step, the author must decide which traversal is the "outer loop" and which the "inner loop", or else we rely on a strategy that splits and rewrites the traversal using the match step logic. So perhaps we can start with the imperative mid-traversal step (this JIRA issue), and then work toward allowing match to optimize multiple sources (a separate JIRA issue).

            This is in the TINKERPOP3-763 branch.

            Open items:

            • Do we add E() support?
            • Add more test cases to GraphTest.
            • Add Graph Step to the documentation – weird.

            dkuppitz – if you want to handle all 3, please do. If you just do the test cases, ping me when you are done and I can do the docs and E()-support. However, we should decide wether we want E() and if we do, can you just add it cause you will need to test it.

            okram Marko A. Rodriguez added a comment - This is in the TINKERPOP3-763 branch. Open items: Do we add E() support? Add more test cases to GraphTest . Add Graph Step to the documentation – weird. dkuppitz – if you want to handle all 3, please do. If you just do the test cases, ping me when you are done and I can do the docs and E() -support. However, we should decide wether we want E() and if we do, can you just add it cause you will need to test it.

            Oh – and dkuppitz – can you also make a GraphStepTest that does that hashCode() stuff you do well. In fact, I think we might need a hashCode() method in GraphStep as it no longer extends StartStep. And we need a reset method.

            okram Marko A. Rodriguez added a comment - Oh – and dkuppitz – can you also make a GraphStepTest that does that hashCode() stuff you do well. In fact, I think we might need a hashCode() method in GraphStep as it no longer extends StartStep . And we need a reset method.
            dkuppitz Daniel Kuppitz added a comment -

            We (that's me and I) don't want E().

            dkuppitz Daniel Kuppitz added a comment - We (that's me and I) don't want E() .
            dkuppitz Daniel Kuppitz added a comment -

            Given the comments in https://issues.apache.org/jira/browse/TINKERPOP3-902, I can just say that I don't want/need E() as a start step nor as a mid-traversal step.

            dkuppitz Daniel Kuppitz added a comment - Given the comments in https://issues.apache.org/jira/browse/TINKERPOP3-902 , I can just say that I don't want/need E() as a start step nor as a mid-traversal step.

            dkuppitz Please do the following.

            1. Add more test cases to GraphStep.
            2. Write the documentation for "Graph Step" (include CAUTION: about indicies).
            3. Write GraphStepTest to verify hashCode() (and you may need to make a hashCode() impl for GraphStep.
            4. Please write TinkerGraphStepStrategyTest and Neo4jGraphStepStrategyTest to ensure they you are getting "has container fold in."

            To point 4, you can probably just copy/paste the test case for TinkerGraphStepStrategyTest to Neo4jGraphStepStrategyTest as the implementation for both is nearly identical. Though, be sure we have both tests as they may evolve independently.

            okram Marko A. Rodriguez added a comment - dkuppitz Please do the following. 1. Add more test cases to GraphStep . 2. Write the documentation for "Graph Step" (include CAUTION: about indicies). 3. Write GraphStepTest to verify hashCode() (and you may need to make a hashCode() impl for GraphStep . 4. Please write TinkerGraphStepStrategyTest and Neo4jGraphStepStrategyTest to ensure they you are getting "has container fold in." To point 4, you can probably just copy/paste the test case for TinkerGraphStepStrategyTest to Neo4jGraphStepStrategyTest as the implementation for both is nearly identical. Though, be sure we have both tests as they may evolve independently.
            dkuppitz Daniel Kuppitz added a comment -

            I just pushed all my changes. You'll probably want to extend the docs section.

            dkuppitz Daniel Kuppitz added a comment - I just pushed all my changes. You'll probably want to extend the docs section.

            You did not complete your work correctly and in full. Please see my comment yesterday on what is required of you for this ticket.

            okram Marko A. Rodriguez added a comment - You did not complete your work correctly and in full. Please see my comment yesterday on what is required of you for this ticket.
            githubbot ASF GitHub Bot added a comment -

            GitHub user dkuppitz opened a pull request:

            https://github.com/apache/incubator-tinkerpop/pull/125

            TINKERPOP3-762: Allow mid-traversal V() (and E())

            • @okram added `V()` as a valid mid-traversal step
            • added traversal test cases
            • added integration tests to verify that indexes will be used in `TinkerGraph` and `Neo4jGraph`
            • added a new `GraphStep` section in the docs

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

            $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP3-762

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

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


            commit 5c5bd06ebbd4ad6781ba46349ebb638f93b4f469
            Author: Marko A. Rodriguez <okrammarko@gmail.com>
            Date: 2015-10-20T20:09:03Z

            GraphStep no longer extends StartStep. It now takes a boolean isStart in its constructor. Moreover, it is no longer a SideEffectStep but a FlatMapStep so it moved to the map/ package. TinkerGraphStepStrateagy and Neo4jGRaphStepStrategy had to be updated to look for ALL GraphStep instances in the traversal and optimize them. Note that @spmallette work in ElementIdStrategy, PartitionStrategy, etc. already did 'for all graph steps' so that didnt need updating. Updated __ and GraphTraversal with V(). I added a new test class called GraphTest with one simple test. I'm passing this off to @dkuppitz to populate GraphTest with compliacated examples. Also, we need to decided if we want mid-traversal E().

            commit 0841bbdc009e4af20fb8bff7899b296eaad3c4da
            Author: Marko A. Rodriguez <okrammarko@gmail.com>
            Date: 2015-10-20T20:26:50Z

            added a reset() step.

            commit 7dd62126c67ade1b59016ab017e6bec7b8e70cc1
            Author: Marko A. Rodriguez <okrammarko@gmail.com>
            Date: 2015-10-20T21:34:06Z

            Merge branch 'master' into TINKERPOP3-762

            commit 6c0e0bd9d0ade9ca62cf1f2a6bdb7a5a3ad0293e
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-25T15:15:09Z

            added a more complex mid-traversal V() traversal test

            commit 7b18447d3641811c291c309180d5688642a8a053
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-27T14:47:02Z

            added another GraphStep traversal test

            commit c35d08f255f4cdd4d498765291a08748423db460
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-27T14:47:42Z

            implemented GraphStep::hashCode()

            commit d83bf71b6ace35f3f0cdf582ba95a67b595fac27
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-27T14:48:44Z

            added a GraphStep core test (hashCode() verification)

            commit a976ca672456590999dfd8d26c7ee27ed558e572
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-27T16:33:01Z

            verified that TinkerGraph and Neo4jGraph fold HasContainers into GraphStep

            commit df443cc5f71904c167ce86149d4275fc8ea6aa67
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-28T11:39:23Z

            added a docs section for GraphStep

            commit 402e57d94fd71f2d652aded37becfe566d4ddad4
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-28T14:32:26Z

            added a srategy test suite for Neo4j

            commit 15d6e7a47bac6609dd890635b568ca05e7862bf0
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-28T14:33:05Z

            Revert "verified that TinkerGraph and Neo4jGraph fold HasContainers into GraphStep"

            This reverts commit a976ca672456590999dfd8d26c7ee27ed558e572.

            commit a71a17983d389be2261a2cf12398c485a25813a7
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-28T15:06:30Z

            mirrored approach to test strategies from neo4j-gremlin into tinkergraph-gremlin

            commit 0900d5f6875ef278a20e6512c1b0f7b77d8586b2
            Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com>
            Date: 2015-10-28T15:16:22Z

            updated CHANGELOG and release doc


            githubbot ASF GitHub Bot added a comment - GitHub user dkuppitz opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/125 TINKERPOP3-762 : Allow mid-traversal V() (and E()) @okram added `V()` as a valid mid-traversal step added traversal test cases added integration tests to verify that indexes will be used in `TinkerGraph` and `Neo4jGraph` added a new `GraphStep` section in the docs You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP3-762 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/125.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 #125 commit 5c5bd06ebbd4ad6781ba46349ebb638f93b4f469 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2015-10-20T20:09:03Z GraphStep no longer extends StartStep. It now takes a boolean isStart in its constructor. Moreover, it is no longer a SideEffectStep but a FlatMapStep so it moved to the map/ package. TinkerGraphStepStrateagy and Neo4jGRaphStepStrategy had to be updated to look for ALL GraphStep instances in the traversal and optimize them. Note that @spmallette work in ElementIdStrategy, PartitionStrategy, etc. already did 'for all graph steps' so that didnt need updating. Updated __ and GraphTraversal with V(). I added a new test class called GraphTest with one simple test. I'm passing this off to @dkuppitz to populate GraphTest with compliacated examples. Also, we need to decided if we want mid-traversal E(). commit 0841bbdc009e4af20fb8bff7899b296eaad3c4da Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2015-10-20T20:26:50Z added a reset() step. commit 7dd62126c67ade1b59016ab017e6bec7b8e70cc1 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2015-10-20T21:34:06Z Merge branch 'master' into TINKERPOP3-762 commit 6c0e0bd9d0ade9ca62cf1f2a6bdb7a5a3ad0293e Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-25T15:15:09Z added a more complex mid-traversal V() traversal test commit 7b18447d3641811c291c309180d5688642a8a053 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-27T14:47:02Z added another GraphStep traversal test commit c35d08f255f4cdd4d498765291a08748423db460 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-27T14:47:42Z implemented GraphStep::hashCode() commit d83bf71b6ace35f3f0cdf582ba95a67b595fac27 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-27T14:48:44Z added a GraphStep core test (hashCode() verification) commit a976ca672456590999dfd8d26c7ee27ed558e572 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-27T16:33:01Z verified that TinkerGraph and Neo4jGraph fold HasContainers into GraphStep commit df443cc5f71904c167ce86149d4275fc8ea6aa67 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-28T11:39:23Z added a docs section for GraphStep commit 402e57d94fd71f2d652aded37becfe566d4ddad4 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-28T14:32:26Z added a srategy test suite for Neo4j commit 15d6e7a47bac6609dd890635b568ca05e7862bf0 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-28T14:33:05Z Revert "verified that TinkerGraph and Neo4jGraph fold HasContainers into GraphStep" This reverts commit a976ca672456590999dfd8d26c7ee27ed558e572. commit a71a17983d389be2261a2cf12398c485a25813a7 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-28T15:06:30Z mirrored approach to test strategies from neo4j-gremlin into tinkergraph-gremlin commit 0900d5f6875ef278a20e6512c1b0f7b77d8586b2 Author: Daniel Kuppitz <daniel_kuppitz@hotmail.com> Date: 2015-10-28T15:16:22Z updated CHANGELOG and release doc
            githubbot ASF GitHub Bot added a comment -

            Github user okram commented on the pull request:

            https://github.com/apache/incubator-tinkerpop/pull/125#issuecomment-151895846

            Ran `mvn clean install` and it worked. +1. Also, I review the test suite developments, docs, and did the mid-traversal `V()` implementation myself.

            githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/125#issuecomment-151895846 Ran `mvn clean install` and it worked. +1. Also, I review the test suite developments, docs, and did the mid-traversal `V()` implementation myself.
            githubbot ASF GitHub Bot added a comment -

            Github user dkuppitz commented on the pull request:

            https://github.com/apache/incubator-tinkerpop/pull/125#issuecomment-152013958

            All the tests there were implemented specifically for the changes in this PR pass for me. However, I'm currently having issues with Giraph and can't get the full integration test suite to pass.

            Hence, if anybody else can confirm, that integration tests still pass, my vote is +1.

            githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/125#issuecomment-152013958 All the tests there were implemented specifically for the changes in this PR pass for me. However, I'm currently having issues with Giraph and can't get the full integration test suite to pass. Hence, if anybody else can confirm, that integration tests still pass, my vote is +1.
            githubbot ASF GitHub Bot added a comment -

            Github user spmallette commented on the pull request:

            https://github.com/apache/incubator-tinkerpop/pull/125#issuecomment-152231933

            code looks tested, docs look good, and `mvn clean install` works on my end - if that is sufficient testing/review then i'm +1

            githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/125#issuecomment-152231933 code looks tested, docs look good, and `mvn clean install` works on my end - if that is sufficient testing/review then i'm +1
            githubbot ASF GitHub Bot added a comment -

            Github user asfgit closed the pull request at:

            https://github.com/apache/incubator-tinkerpop/pull/125

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

            People

              dkuppitz Daniel Kuppitz
              dkuppitz Daniel Kuppitz
              Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: