You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@tinkerpop.apache.org by "dkuppitz (GitHub)" <gi...@apache.org> on 2019/01/15 17:11:06 UTC

[GitHub] [tinkerpop] dkuppitz opened pull request #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

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

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

VOTE +1

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] dkuppitz commented on pull request #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "dkuppitz (GitHub)" <gi...@apache.org>.

I just put some breakpoints on the lines that mutate the traversal, but it looks like the tests in the provider test suite all have pretty optimal traversals, hence this strategy never changes any of them.

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on pull request #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "spmallette (GitHub)" <gi...@apache.org>.

could you add some javadoc like our other strategies:

https://github.com/apache/tinkerpop/blob/e51bfc90fe6a863c17016c80fe53b4b5ccb97fd0/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java#L50-L62

actually, a bit more "explaining" inline javadoc in the code would be nice too - would be nice to get a feel for why the code is doing what it is doing at each major step without having to try to understand it all.

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on pull request #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "spmallette (GitHub)" <gi...@apache.org>.

This is a pretty nice batch of test.  Still thinking if there are cases that are not covered....

Are there any existing provider test suite tests that trigger this strategy?

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on pull request #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "spmallette (GitHub)" <gi...@apache.org>.

could you add some javadoc like our other strategies:

https://github.com/apache/tinkerpop/blob/e51bfc90fe6a863c17016c80fe53b4b5ccb97fd0/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java#L50-L62

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on issue #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "spmallette (GitHub)" <gi...@apache.org>.

please give me a little extra time to review this one - i had my head in other things last week.

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on issue #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "spmallette (GitHub)" <gi...@apache.org>.

Other than the few minor comments I made, I think this looks good. You have 100% coverage on the new strategy which is cool. I can't help thinking that there is yet some case that hasn't be covered but I'm not sure what case of significance isn't in your tests already.  

Only thing I could think of was maybe a test or two that has the `range()` as an inner traversal? not sure I noticed that in your set of tests and it wouldn't offer any additional coverage, but perhaps it still has value?

I think it's worth mentioning this strategy in the upgrade docs and explaining its value.

VOTE +1

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on pull request #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "spmallette (GitHub)" <gi...@apache.org>.

This is a pretty nice batch of test.  Still thinking if there are cases that are not covered....

Are there any provider test suite tests that trigger this strategy?

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on pull request #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "spmallette (GitHub)" <gi...@apache.org>.

Might want to add one or two specifically for this strategy just so that it gets exercised on graph provider systems.

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] dkuppitz closed pull request #1040: TINKERPOP-1882 Apply range and limit steps as early as possible

Posted by "dkuppitz (GitHub)" <gi...@apache.org>.

[ pull request closed by dkuppitz ]

[ Full content available at: https://github.com/apache/tinkerpop/pull/1040 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org