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

Provide String-based withStrategy()/withoutStrategy() for language variant usage

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.2
    • Fix Version/s: 3.2.3
    • Component/s: language-variant, process
    • Labels:
      None

      Description

      Right now withStrategies() is not supported by Gremlin-Python. Why? Because strategies are created via Java and thus you can't do stuff like:

      g.withStrategies(SubgraphStrategy.build()...create())
      

      Now, we have strategies that we have made "native" to GraphTraversalSource by way of withXXX. For example: withSideEffect(), withSack(), withRemote(), withPath(), etc.

      In order to generally support any strategy created by a user for use from a language variant, we should support lambda based strategies. E.g.:

      g.withStrategies(lambda : "SubgraphStrategy.build()...create()")
      

      Like any other lambda, it is executed server side using the respective ScriptEngine.

      Next, withoutStrategies() should support String... so you can do:

      g.withoutStrategies("com.stuff.MyStrategy", "com.stuff.MyOtherStrategy")
      

      ..instead of having to pass in the Class object.

        Issue Links

          Activity

          Hide
          okram Marko A. Rodriguez added a comment -

          After some more thinking, I think it would be like this:

          g.withStrategies("org.apache.tinkerpop...SubgraphStrategy", "vertices", has("age"), "edges", inV().has("age",gt(10)))
          

          So what do we have here?

          1. This is language agnostic but NOT VM agnostic. That is "okay."
          2. Strategies will need to support a static Strategy.create(Configuration) or Strategy.create(Object...) method.
          3. Strategies should support a strategy.getConfiguration() method.

          With this, we can then expose withStrategies() to all Gremlin language variants as it is no longer Java bound.

          Show
          okram Marko A. Rodriguez added a comment - After some more thinking, I think it would be like this: g.withStrategies( "org.apache.tinkerpop...SubgraphStrategy" , "vertices" , has( "age" ), "edges" , inV().has( "age" ,gt(10))) So what do we have here? 1. This is language agnostic but NOT VM agnostic. That is "okay." 2. Strategies will need to support a static Strategy.create(Configuration) or Strategy.create(Object...) method. 3. Strategies should support a strategy.getConfiguration() method. With this, we can then expose withStrategies() to all Gremlin language variants as it is no longer Java bound.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user okram opened a pull request:

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

          TINKERPOP-1455: Provide String-based withStrategy()/withoutStrategy() for language variant usage

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

          Added TraversalSource.withStrategy(String, Object...) and TraversalSource.withoutStrategy(String). This allows Gremlin language variants (e.g. Gremlin-Python) to add and remove strategies from a traversal source. Prior to this moment, strategy addition/removal was via strategy instances (Java objects) and classes (Class objects). That is not language variants friendly.

          This branch is backwards compatible. If a `TraversalStrategy` does not have a builder and exposes a static `instance()`-method, then `g.withStrategy('com.blah.MyStrategy')` will work as expected. If a `TraversalStrategy` does have a builder (that is, can be configured), then it should expose a static `create(Configuration)`-method. Note Gremlin language variants could not use `withStrategies()` prior to this and thus, this addition is backwards compatible. Finally, `TraversalSource.withComputer(Object...)` was added as well for the same reasons.

          ```
          >>> g = g.withStrategy('org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.SubgraphStrategy','vertices',hasLabel('person'),'edges',has('weight',gt(0.5)))
          >>> g.V().name.toList()
          [u'marko', u'vadas', u'josh', u'peter']
          ```

          The CHANGELOG, gremlin-variants.asciidoc, and upgrade doc have all been updated accordingly.

          VOTE +1

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

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

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

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


          commit e6e59e417f119c1b71a43141fce04cb212bc0a58
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-10-04T14:37:07Z

          first push. We now have TraversalSource.withStrategy(String, Object...) and TraversalSource.withoutStrategy(String). Added a test case to Gremlin-Python that uses SubgraphStrategy and it works .

          commit 95557bf3aac279282e3f3779d68ce195bd4ca058
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-10-04T16:26:12Z

          added static create(Configuration) methods to all Builder-based strategies except EventStrategy as that requires Java objects . Deprecated PartitionStrategy.Builder.addReadParition() in favor of readParitions(String...). Computer now has a Configuration-based constructor and corresponding withComputer(Object...) arguments.

          commit 20dec2076b79441c7dcf06827014ace72fcaea6c
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-10-04T18:15:31Z

          updated gremlin-variants.asciidoc with new withStrategy()/withoutStrategy() method discussion. Built docs. Success.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user okram opened a pull request: https://github.com/apache/tinkerpop/pull/447 TINKERPOP-1455 : Provide String-based withStrategy()/withoutStrategy() for language variant usage https://issues.apache.org/jira/browse/TINKERPOP-1455 Added TraversalSource.withStrategy(String, Object...) and TraversalSource.withoutStrategy(String). This allows Gremlin language variants (e.g. Gremlin-Python) to add and remove strategies from a traversal source. Prior to this moment, strategy addition/removal was via strategy instances (Java objects) and classes (Class objects). That is not language variants friendly. This branch is backwards compatible. If a `TraversalStrategy` does not have a builder and exposes a static `instance()`-method, then `g.withStrategy('com.blah.MyStrategy')` will work as expected. If a `TraversalStrategy` does have a builder (that is, can be configured), then it should expose a static `create(Configuration)`-method. Note Gremlin language variants could not use `withStrategies()` prior to this and thus, this addition is backwards compatible. Finally, `TraversalSource.withComputer(Object...)` was added as well for the same reasons. ``` >>> g = g.withStrategy('org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.SubgraphStrategy','vertices',hasLabel('person'),'edges',has('weight',gt(0.5))) >>> g.V().name.toList() [u'marko', u'vadas', u'josh', u'peter'] ``` The CHANGELOG, gremlin-variants.asciidoc, and upgrade doc have all been updated accordingly. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1455 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/447.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 #447 commit e6e59e417f119c1b71a43141fce04cb212bc0a58 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-10-04T14:37:07Z first push. We now have TraversalSource.withStrategy(String, Object...) and TraversalSource.withoutStrategy(String). Added a test case to Gremlin-Python that uses SubgraphStrategy and it works . commit 95557bf3aac279282e3f3779d68ce195bd4ca058 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-10-04T16:26:12Z added static create(Configuration) methods to all Builder-based strategies except EventStrategy as that requires Java objects . Deprecated PartitionStrategy.Builder.addReadParition() in favor of readParitions(String...). Computer now has a Configuration-based constructor and corresponding withComputer(Object...) arguments. commit 20dec2076b79441c7dcf06827014ace72fcaea6c Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-10-04T18:15:31Z updated gremlin-variants.asciidoc with new withStrategy()/withoutStrategy() method discussion. Built docs. Success.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

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

          ```
          [INFO] ------------------------------------------------------------------------
          [INFO] BUILD SUCCESS
          [INFO] ------------------------------------------------------------------------
          [INFO] Total time: 05:09 h
          [INFO] Finished at: 2016-10-04T22:33:42-06:00
          [INFO] Final Memory: 157M/1520M
          [INFO] ------------------------------------------------------------------------
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/447 ``` [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 05:09 h [INFO] Finished at: 2016-10-04T22:33:42-06:00 [INFO] Final Memory: 157M/1520M [INFO] ------------------------------------------------------------------------ ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

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

          Code looks good, `docker/build.sh -t -i` succeeded.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/447 Code looks good, `docker/build.sh -t -i` succeeded. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

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

          This is a great PR. Didn't think we'd have this for 3.2.3. Nice

          All tests pass with `docker/build.sh -t -i` - final "todo" would be to add a JIRA to track the deprecation for future removal.

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/447 This is a great PR. Didn't think we'd have this for 3.2.3. Nice All tests pass with `docker/build.sh -t -i` - final "todo" would be to add a JIRA to track the deprecation for future removal. VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

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

          If you guys don't mind, I'm going to change the method signatures (the NEW methods added – nothing from before changed).

              withStrategy(String, Object...) -> withStrategies(Map<String,Object>...)
              withoutStrategy(String) -> withoutStrategies(String...)
              

          The reason this is smart is:

          1. Registering strategies is expensive as it costs a `TraversalSource` clone and a `TraversalStrategies` sort. This is why doing all strategy declarations in one method is smart.

          2. While creating `Map`s in Java is horrendous, Java users would NOT use these methods. They would use the Java object version of these methods. However, for Gremlin-Python, Groovy, JavaScript – creating maps is natural and looks good. Also, we don't run into the "multi-key" issues.

          Nothing fudamental here, just some asethetics. If you don't object, I will just do this, update docs, and merge.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/447 If you guys don't mind, I'm going to change the method signatures (the NEW methods added – nothing from before changed). withStrategy( String , Object ...) -> withStrategies(Map< String , Object >...) withoutStrategy( String ) -> withoutStrategies( String ...) The reason this is smart is: 1. Registering strategies is expensive as it costs a `TraversalSource` clone and a `TraversalStrategies` sort. This is why doing all strategy declarations in one method is smart. 2. While creating `Map`s in Java is horrendous, Java users would NOT use these methods. They would use the Java object version of these methods. However, for Gremlin-Python, Groovy, JavaScript – creating maps is natural and looks good. Also, we don't run into the "multi-key" issues. Nothing fudamental here, just some asethetics. If you don't object, I will just do this, update docs, and merge.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

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

          no problem from me.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/447 no problem from me.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

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

          Cool.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/447 Cool.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

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

          ```
          [INFO] ------------------------------------------------------------------------
          [INFO] BUILD SUCCESS
          [INFO] ------------------------------------------------------------------------
          [INFO] Total time: 05:10 h
          [INFO] Finished at: 2016-10-06T02:35:44-06:00
          [INFO] Final Memory: 158M/1358M
          [INFO] ------------------------------------------------------------------------
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/447 ``` [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 05:10 h [INFO] Finished at: 2016-10-06T02:35:44-06:00 [INFO] Final Memory: 158M/1358M [INFO] ------------------------------------------------------------------------ ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user okram commented on the issue:

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

          So this is now done and merged. This branch provided a wealth of testing infrastructure, test cases, and ways of serializing `TraversalStrategy` instances. Now, with all that, I think we should actually NOT have `withStrategies(Map...)` and we should really make `g:TraversalStrategy`. I started going down that road to see how easy it would be and its actually pretty hard given how `GraphSONMapper` works – and the need to register all strategies by class. Anywho... if we do decide to go down that route then in Gremlin-Python/etc. we would have `TraversalStrategy(

          {config:map,is:this}

          )`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/447 So this is now done and merged. This branch provided a wealth of testing infrastructure, test cases, and ways of serializing `TraversalStrategy` instances. Now, with all that, I think we should actually NOT have `withStrategies(Map...)` and we should really make `g:TraversalStrategy`. I started going down that road to see how easy it would be and its actually pretty hard given how `GraphSONMapper` works – and the need to register all strategies by class. Anywho... if we do decide to go down that route then in Gremlin-Python/etc. we would have `TraversalStrategy( {config:map,is:this} )`.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development