I assume you are referring to these booleans: I should have written “conditionals checking the SearchAlgo“. Like these <https://github.com/apache/tinkerpop/pull/838/files#diff-f4f092baa85e1e17a21ee21ea7f0c8f3R320> which for now I have defaulting to BFS <https://github.com/apache/tinkerpop/pull/838/files#diff-f4f092baa85e1e17a21ee21ea7f0c8f3R48>
I’m not sure that I like the overload of order() as the method for doing that I have no strong opinions on overloading order. I do like the idea of using something similar to emit and until though. I still need to study your changes in detail. Sure, I don’t know if a quick explanation will help, but here it is anyway. The biggest chunk of it is really around those conditionals I mentioned. The starts are stashed here <https://github.com/apache/tinkerpop/pull/838/files#diff-f4f092baa85e1e17a21ee21ea7f0c8f3R213> or here <https://github.com/apache/tinkerpop/pull/838/files#diff-f4f092baa85e1e17a21ee21ea7f0c8f3R340> for DFS and are then accessed through the nextStart method here <https://github.com/apache/tinkerpop/pull/838/files#diff-f4f092baa85e1e17a21ee21ea7f0c8f3R191> That’s really how the DFS vs BFS works for this PR, the rest is adding the order step overload and SearchAlgo enum. On Tue, Apr 17, 2018 at 8:11 AM Stephen Mallette <spmalle...@gmail.com> wrote: Thanks for the summary. Just a quick note - I'd not worry about the GLV > tests for now. That part should be easy to sort out. Let's first make sure > that we get clear on the other items first before digging too deeply there. > > On an administrative front, I think that this change should just go to > 3.4.0/master (so it's currently targeted to the right branch actually) as > it sounds like we want DFS to be the default and that could be a breaking > change as the semantics of the traversal shift a bit. So that's one issue > to make sure we're all happy with. > > A next issue would be the API from the user perspective - thanks for the > link to that JIRA btw - I see I was involved in that conversation a little > bit but then dropped off. You'd commented that you preferred a per loop > configuration for search approach which is what you stuck with for the > implementation. I guess I can see that there could be some value in having > that ability. That said, I'm not sure that I like the overload of order() > as the method for doing that: > > g.V().has("name", "marko"). > repeat(__.outE().order().by("weight", decr).inV()). > emit(). > order(SearchAlgo.DFS) > > Still thinking about what else we might do there, but perhaps that can wait > a bit as I still need to study your changes in detail. Regarding: > > > The barrier step that Daniel described doesn’t currently work since > there’s basically booleans in the RepeatStep on whether or not to stash the > starts to make the RepeatStep depth first. > > I assume you are referring to these booleans: > > > https://github.com/apache/tinkerpop/blob/fe8ee98f6967c7b0f0ee7f2cf2d10531b68fab8b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java#L46-L47 > > ? > > > > On Tue, Apr 17, 2018 at 7:37 AM, Keith Lohnes <lohn...@gmail.com> wrote: > > > Stephen, That’s a fair summary. I had an immediate need for it, so I > > developed something based on Michel Pollmeier’s work and a modification > to > > the syntax Pieter Martin suggested in the Jira > > <https://issues.apache.org/jira/browse/TINKERPOP-1822? > > focusedCommentId=16233681&page=com.atlassian.jira. > > plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16233681> > > with DFS being explicit. > > > > The semantics I used were g.V().repeat(out()).order(DFS), putting the > > order > > clause outside of the repeat. It made it simpler for me to develop and > > seemed nice to make that DFS vs BFS explicit. The barrier step that > Daniel > > described doesn’t currently work since there’s basically booleans in the > > RepeatStep on whether or not to stash the starts to make the RepeatStep > > depth first. > > > > I added a test for the DFS, though I’ve had some issues getting things to > > run re: .net tests and some other tests timing out. I have some more > tests > > I'd like to write based off issues that I ran into testing this in the > > console (k-ary trees, until/emit before the repeat vs after), but I > really > > wanted to get this out there for people to take a look at and see if it > > could work out. > > > > > > On Mon, Apr 16, 2018 at 7:29 PM Stephen Mallette <spmalle...@gmail.com> > > wrote: > > > > > Keith, I have to admit that I didn't really follow this general body > of > > > work closely, which include this pull request, the earlier one from > > Michael > > > Pollmeier, the JIRA and I think some discussion somewhere on one of the > > > mailing lists. As best I have gathered from what I did follow, I feel > > like > > > the focus of this work so far was mostly related to "can someone get it > > to > > > actually work", but not a lot about other aspects like testing, api, > > > release branch to apply it to, etc. Is that a fair depiction of how > this > > > work has developed so far? if so, let's use this thread to make sure > > we're > > > all on the same page as to how this change will get in on all those > sorts > > > of issues. > > > > > > btw, thanks to you and michael for sticking at this to come to > something > > > that seems to work. looking forward to the continued discussion on this > > > thread. > > > > > > > > > On Mon, Apr 16, 2018 at 6:54 PM, Michael Pollmeier < > > > mich...@michaelpollmeier.com> wrote: > > > > > > > Unsurprisingly I'm also +1 for defaulting to DFS in OLTP. My feeling > is > > > > that most users currently expect it to be DFS since that's what the > > docs > > > > say. > > > > > > > > And yes, it's easy to verify the default in the test suite, once we > > > > agreed on what the default should be. > > > > > > > > Cheers > > > > Michael > > > > > > > > On 17/04/18 04:40, pieter gmail wrote: > > > > > Hi, > > > > > > > > > > I have not properly followed the previous thread. However I thought > > is > > > > > going to be a way to set the default behavior as apposed to needing > > to > > > > > use barrier() > > > > > Is this the case or not? > > > > > > > > > > For Sqlg at least it is possible to optimize BFS much more > > effectively > > > > > than DFS so it will be nice to have a way to set the strategy > rather > > > > > than having to manually inject barriers. > > > > > > > > > > Is the test suite going to enforce the BFS vs DFS? > > > > > > > > > > Thanks > > > > > Pieter > > > > > > > > > > On 16/04/2018 16:56, Daniel Kuppitz wrote: > > > > >> +1 for DFS. If the query relies on BFS, you can always do > > > > >> .repeat(....barrier())... > > > > >> > > > > >> ^ This holds true as long as there's no significant difference in > > the > > > > >> cpu+memory consumption and overall performance of the two > > approaches. > > > > BFS > > > > >> has its advantages when it comes to bulking; an arbitrary number > of > > > > >> traversers on the same element is processed at the same pace as a > > > single > > > > >> traverser. I don't think we can benefit from bulking in DFS. > > > > >> > > > > >> Cheers, > > > > >> Daniel > > > > >> > > > > >> > > > > >> On Mon, Apr 16, 2018 at 5:44 AM, Keith Lohnes <lohn...@gmail.com> > > > > wrote: > > > > >> > > > > >>> As part of #838 <https://github.com/apache/tinkerpop/pull/838> > > > there’s > > > > >>> some > > > > >>> discussion around whether or not to make DFS the default for the > > > repeat > > > > >>> step. On the one hand, everything else in OLTP is depth first. On > > the > > > > >>> other > > > > >>> hand, there’s likely existing traversals that depend on the > breadth > > > > >>> first > > > > >>> nature of repeat. My general preference is to make DFS optional > at > > > > >>> first, > > > > >>> and at some later date, change the default and have that be a > > > separate > > > > >>> change from implementing DFS for repeat > > > > >>> > > > > >>> > > > > > > > > > > > > > > > > > > > > > > > >
