Fix CountStrategy returning capped counts when the count is observed

[upadhyay-prashant]

Summary

CountStrategy rewrites count().is(P) to limit(n).count().is(P). When P accepts n, the capped value is emitted in place of the real count, so anything that observes it gets a wrong result:

• g.V().where(outE().count().is(gt(1)).is(lt(3))) keeps vertices with more than 2 edges
• g.V().outE().count().is(gt(2)) returns 3 instead of 6
• g.V().map(outE().count().is(gt(1))) returns 2 instead of 3

The limit is now only added when:

1. P rejects the cap (e.g. lt, lte, within), so any count that passes is exact; or
2. the count is used purely as a filter (where/filter, not, and/or, sideEffect, repeat().until()/emit()) and no step follows is(). A label at the end of a where() child adds a WhereEndStep that compares the count to the path, so that case is not limited.

The strategy also leaves count().is(P) alone when P contains a traversal (e.g. is(gt(1).and(lt(__.constant(3))))). That bound is only resolved at runtime, so no safe limit can be derived. This was already broken before this PR: filter(outE().count().is(gt(1).and(lt(__.constant(3))))) returned marko as well as josh, and outE().count().as("a").is(gt(1).and(lte(__.select("a")))) returned 2 instead of 6. A narrower rule (keep the limit when an and() has a literal part that rejects the cap) is possible as a follow-up.

Otherwise the traversal is left unchanged. The existing not() / filter-removal rewrites are untouched.

Testing

• CountStrategyTest: added cases covering shapes that must not be limited and shapes that must stay optimized; every case whose expectation changed has a still-limited counterpart (e.g. out().count().is(neq(4)) vs where(out().count().is(neq(4)))). Also covers traversal-valued predicates in root, filter-only, label-reading and not() forms.
• LazyBarrierStrategyTest / TraversalExplanationTest: updated two expectations that relied on the old root-level / by() rewrite; LazyBarrierStrategyTest also gained a where() counterpart.
• CountStrategy.feature: added 6 scenarios, all of which produced wrong results before the fix (except the lt(3).is(gt(1)) control) and pass after.
• Full gremlin-core suite and tinkergraph-gremlin (including its feature suites): 0 failures.
• Regenerated translations.json and the Go/Python/.NET/JS translation files (additions only). GLV feature tests were not run locally.

Notes

• Root-level and value-consuming usages such as count().is(gt(n)) no longer get the early limit(). That is required for correctness.
• This bug likely also exists on 3.7-dev/3.8-dev; this PR targets master.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.77%. Comparing base (a28cd1f) to head (2865a74).
⚠️ Report is 733 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3717      +/-   ##
============================================
+ Coverage     76.35%   76.77%   +0.41%     
- Complexity    13424    14365     +941     
============================================
  Files          1012     1037      +25     
  Lines         60341    64834    +4493     
  Branches       7075     7708     +633     
============================================
+ Hits          46076    49774    +3698     
- Misses        11548    11970     +422     
- Partials       2717     3090     +373     

☔ View full report in Codecov by Harness.
📢 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.

[Cole-Greer]

I find it surprising that the changes result in this case dropping the limit(), but the limit is retained in all of the above cases:

{__.count().is(-2023), __.limit(0).count().is(-2023)},
{__.count().is(P.between(0, 10)), __.limit(10).count().is(P.between(0, 10))},
{__.count().is(P.between(5, 10)), __.limit(10).count().is(P.between(5, 10))},

My expectation would be that the limit no longer gets added in all of these basic count().is(...) cases.

[upadhyay-prashant]

The difference is whether the capped count can pass the predicate. If it can't, any count that does pass is below the cap and therefore exact, so the limit is harmless even at the root:

• is(-2023): limit(0) gives a count of 0, which fails the predicate (and a count can never be negative anyway).
• between(0, 10) / between(5, 10): the upper bound is exclusive, so the cap of 10 fails.
• gt(10).or(lt(5)): the cap of 11 passes gt(10), so a traversal of 20 elements would return 11. That's the case the PR fixes.

Dropping the limit for every root-level case would be simpler, but it would throw away optimizations that are always safe. I've added a comment above these cases in the test to explain this.

[Cole-Greer]

For each test case we are updating by removing the limit(), I think I'd like to reintroduce a new case with a similar traversal and predicate, but where we still expect a limit to be added. In this case, it could be something like this:

{__.out().where(__.count().is(neq(4))), __.out().where(__.limit(5).count().is(neq(4)))},

I'd like to retain decent coverage of "positive" cases where CountStrategy is still productive following this change.

[upadhyay-prashant]

Agreed, and done. Every case whose expectation changed now has a counterpart that is still limited:

• out().count().is(neq(4)) -> where(out().count().is(neq(4))) and your out().where(count().is(neq(4))), both limit(5)
• is(gt(2)) -> filter(out().count().is(gt(2))), limit(3)
• is(gte(2)) -> filter(out().count().is(gte(2))), limit(2)
• is(outside(2, 4)) -> where(out().count().is(outside(2, 4))), limit(5)
• is(gt(10).or(lt(5))) -> where(count().is(gt(10).or(lt(5)))), limit(11)
• LazyBarrierStrategyTest root is(gt(10)) -> a where() version that keeps limit(11)

[Cole-Greer]

I may be missing some important detail here, but in my mind this sort of where()/filter() nesting is the classic example in which the limit() injection optimization is safe. The where()/filter() steps are only testing whether the child traversal is productive, so the specific value of the returned count has no impact on if the filter is true or false.

[upadhyay-prashant]

You're right. Treating labels as "observed" was overly conservative, and I've removed that check. In a filter-only child, labels can't be read from outside it, so where(out().count().as("a").is(gt(1))) and both filter(...) label cases now get the limit.

The one exception is a label at the end of the where() child, e.g. where(out().count().is(gt(1)).as("a")). That makes where() build a WhereTraversalStep, which appends a WhereEndStep that compares the count to the path value "a", so the capped value would be observed. That case is still caught by the existing "a step follows is()" check, so no label-specific logic is needed.

The cases that still must not be limited inside where()/filter() are the ones with a following is(), e.g. where(out().count().is(gt(1)).is(lt(3))). The second is() sees the capped value. The new CountStrategy.feature scenarios show the wrong results this caused on the modern graph.

Looking at where a label could actually be read from also turned up one more case: the is() predicate itself can hold a traversal (P.gt(__.constant(29))), and that traversal runs inside the child. For example, filter(outE().count().is(gt(1).and(lt(__.constant(3))))) returned marko as well as josh, and outE().count().as("a").is(gt(1).and(lte(__.select("a")))) returned 2 instead of 6. The strategy only sees the literal part of the predicate, so it can't derive a safe limit. CountStrategy now leaves count().is(P) alone whenever P contains a traversal. This was already broken before this PR, but it's the same class of bug, so I've included the fix and two feature scenarios for it here. A narrower rule (keep the limit when an and() has a literal part that rejects the cap) would be possible as a follow-up, but it needs careful handling of and/or/not with unknown values, so I kept it simple here.