[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user GCHQResearcher1337 commented on the issue: https://github.com/apache/tinkerpop/pull/876 @spmallette Yep. Awesome - thanks for all your input + reviews! ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/876 I will take care of merging this now that we have reviews done. Thanks for patiently working through this process @GCHQResearcher1337 - is that how you will forever be known to us by the way? :smile: ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/876 VOTE: +1 ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user GCHQResearcher1337 commented on the issue: https://github.com/apache/tinkerpop/pull/876 @dkuppitz Thanks for your comments. I think I've fixed the issues you spotted. ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/876 VOTE +1 ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/876 Nice @GCHQResearcher1337 - VOTE +1 ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user GCHQResearcher1337 commented on the issue: https://github.com/apache/tinkerpop/pull/876 I've now added support for named loops - described in https://issues.apache.org/jira/browse/TINKERPOP-967, however I have not introduced a new `times()` step. I think a new `times()` step would be of marginal benefit and could be confusing - currently `times()` is a step modulator on `repeat()`. I've also changed to explicit loop stack initialisation rather than setting it up on the increment. Setting up the stack/counter on the increment creates a problem in the nested context - when the inner `loops()` is called it incorrectly returns the top counter from the stack. ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/876 I agree it should be treated differently. Since `as()` labeling has an impact on the path history, it really shouldn't be used for this case. ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/876 > perhaps there is a reason to unite the two concepts. can't really think of a reason to "unite the two concepts" - I think that we want to keep it as `repeat(âaâ,â¦) != repeat(â¦).as(âaâ)`. @dkuppitz any thoughts on that? ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/876
Some more notes from Marko (i haven't had a chance to let them sink in
myself, just pasting them here):
```text
times(a,2) == loops(a,eq(2))
```
the idea being, you might want to check on the loop count of an outer loop
within an inner loop and vice versa. Basically, you are trying to solve:
```text
for(i=0;i<10) { for(j=0;j<10) if(i == x) â¦. } â¦.
```
the point being, you want to be able to check for `i` and `j` counters not
just at the loop check point but anywhere in the respective body.
And note that naming `repeat()`s is not the same as naming the
`repeat()`-step. It isnât a step labelâ¦â¦â¦ though, some thinking on that
might be worth it. In essence `repeat(âaâ,â¦) != repeat(â¦).as(âaâ)`
the âaâ in the first is for the loop stack, not for the step
labelâ¦.but again, perhaps there is a reason to unite the two concepts. Though
if done so, a new syntax would be introduced which would add an
âirregularityâ to Gremlin
---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user GCHQResearcher1337 commented on the issue:
https://github.com/apache/tinkerpop/pull/876
@spamallete I think I steered away from that because I initially
misunderstood what marko meant - as you pointed out the plumbing (`incrLoops`
taking a `stepLabel`) for nested repeats is already there, but I didn't find
any plumbing had been done for this (neither `times()` nor `loop()` can take a
`stepLabel`) so I thought that the 'work on `LoopStep`' had been abandoned.
Re-reading this I think I see what is involved and can implement this -
both `TimesModulating` and `LoopStep` as well as the `loops()` method of a
traverser need to be able to take a stepLabel. We would need to maintain a
separate user-defined stepLabel that could be accessed.
I can see why `loops('a')` could be helpful, but I don't know why you would
want to do `times('a',2)`. Could you please provide an example of this?
PS Hi Marko! Thanks for Gremlin!
---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/876
@GCHQResearcher1337 any reason you didn't implement the labelling for
`repeat()` as described in the JIRA issue:
```text
repeat('a',out('knows').repeat('b',out('parent')))
```
As the issue states, it's usage would probably be rare, but marko had that
intention when he created the ticket so I thought I'd ask about it (also marko
has been lurking about and mentioned it to me last night :smile: ).
---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/876 @GCHQResearcher1337 I should have also added in my last comment: 1. Please add [CHANGELOG ](https://github.com/apache/tinkerpop/blob/master/CHANGELOG.asciidoc) entries as necessary to 3.4.0 2. This feature is sufficiently interesting that it probably deserves some special attention. Do you mind writing up something to introduce this change to users in the [Upgrade Documentation](https://github.com/apache/tinkerpop/blob/master/docs/src/upgrade/release-3.4.x.asciidoc) for 3.4.0. If you like, I can handle the upgrade docs for you when we go to merge. Just let me know how you'd like to proceed. ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/876 This PR shows a pretty nice level of understanding of the intricacies of the core of how Gremlin works. We rarely get PRs that touch this area of TinkerPop to any depth. I found it interesting that most of the plumbing for this feature was already there. It mostly just needed a new `Traverser` species in place to make it all come together. I made a separate comment on this PR to expand a unit test case, but other than that, it looks pretty good. As a minor nit, @GCHQResearcher1337 if you could please make a second pass through your changes and `final` any variables you see that can be marked as such (as that is our style). @dkuppitz could you take a look at the Gremlin tests added here in detail and see if you would recommend any others? Anyway, that's my initial pass at this. I expect to give it a second look before I provide my VOTE. I would also like to see feedback from @dkuppitz who is on holiday until next week, so I probably won't have any other feedback until then. ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/876 wow - this is a cool pull request. it may take some time to go through review as it is a non-trivial change. thanks. ---
