Change range in repeat to track counters per iteration

[andreachild]

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

This changeset aligns the semantics of traversals with range() and limit() used in repeat() with the behaviour of the equivalent 'unrolled' traversal without repeat(). This is done by tracking per-iteration counters instead of a global counter. The semantics of range() and limit() without repeat() and with local() were not modified.

Single and nested loop Traverser implementations were also refactored to reduce much code duplication by consolidating common loop logic into a new NL_SL_Traverser base interface for loop-aware traversers.

VOTE +1

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (3.8-dev@f2bd35e). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff             @@
##             3.8-dev    #3241   +/-   ##
==========================================
  Coverage           ?   75.71%           
==========================================
  Files              ?       28           
  Lines              ?     6390           
  Branches           ?        0           
==========================================
  Hits               ?     4838           
  Misses             ?     1331           
  Partials           ?      221           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[xiazcy]

Not sure if we also want to highlight this in the Provider section, but we should add an entry into the gremlin-semantics doc on this.

[Cole-Greer]

This check should probably apply recursively to grandparents etc. Otherwise nesting a RangeGlobalStep inside a global parent leads to failure to "reset" on each iteration:

gremlin> g.V().repeat(limit(1)).times(2)
==>v[1]
gremlin> g.V().repeat(union(limit(1))).times(2)
gremlin> 

[Cole-Greer]

I'm curious if we can get away with dropping this if statement altogether and always returning false instead of throwing a FastNoSuchElementException regardless of if this step is nested in a repeat. A quick test shows that with the exception of g_V_playlist_paths in Paths.feature (which seems to hang indefinitely), all of the remaining feature tests are unaffected by such a change. I'm not quite sure why that one scenario is failing, but that may be a path to a preferable solution if it can be resolved.

It's notable that most filter steps (especially TailGlobalStep) don't throw NoSuchElementExceptions and instead solely rely on returning false from filter() to indicate a result should be filtered.

[andreachild]

I've changed the check to move upwards until the root and added feature test for limit in union in repeat. I prefer to leave the check in to reduce potential backwards compatibility breakage since we know something is going wrong with the Paths.feature test when it is removed.

[Cole-Greer]

I'm a bit confused by this sentence (may be due to my limited understanding of loops tracking in Traversers). I'm not understanding when a Traverser would support nested loops but not single loops. Just looking at the names of the traversers, all of the NL traversers also contain SL. Additionally, the way RepeatStep adds requirements, any Traversal with NL requirements will always contain SL requirements as well.

[xiazcy]

I think it's b/c even though NL traversers contain SL, implementation wise they extend but overrode the SL logic, so essentially it is one or the other?

[andreachild]

I think it's b/c even though NL traversers contain SL, implementation wise they extend but overrode the SL logic, so essentially it is one or the other?

This is exactly why. The NL traversers all extend SL traversers but override all loops related methods to reference the nested loop stack and the single loop counter is essentially unused.

Yes all NL traversers do support SL but do so using the NL stack data structure, ignoring the SL loop counter.

I've modified the javadoc to be hopefully less confusing.

[Cole-Greer]

Do you think there would be value in giving this graph a name and adding it to TinkerFactory such that it is reusable elsewhere, or is it too specific for the exact behaviour you are trying to test here?

[andreachild]

It is specific to this test as I wanted a very predictable structured data to traverse through. For feature tests and local testing the grateful dead graph can be used instead.

[xiazcy]

Could there potentially be performance impacts with deeply nested and complex repeat queries? I don't have any example to offer, but just wondering for awareness.

[andreachild]

In general yes nested repeats can be less performant than the unrolled equivalent but that was the case even prior to this change. I believe that is the reason for creating the RepeatUnrollStrategy which will optimize by unrolling simple repeat traversals if possible.