upadhyay-prashant commented on code in PR #3717:
URL: https://github.com/apache/tinkerpop/pull/3717#discussion_r4290151978
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java:
##########
@@ -71,7 +71,7 @@ public static Iterable<Object[]> generateTestParameters() {
{__.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))},
- {__.count().is(P.gt(10).or(lt(5))),
__.limit(11).count().is(P.gt(10).or(lt(5)))},
+ {__.count().is(P.gt(10).or(lt(5))),
__.count().is(P.gt(10).or(lt(5)))}, // capped count of 11 would pass and be
returned
Review Comment:
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.
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java:
##########
@@ -84,15 +84,46 @@ public static Iterable<Object[]> generateTestParameters() {
{__.filter(__.out().out().count().is(0)),
__.not(__.out().out())},
{__.local(aggregate("x")).count().is(0).as("a"),
__.local(aggregate("x")).limit(1).count().is(0).as("a")},
{__.out().count().as("a").is(0),
__.out().limit(1).count().as("a").is(0)},
- {__.out().count().is(neq(4)),
__.out().limit(5).count().is(neq(4))},
Review Comment:
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)`
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java:
##########
@@ -84,15 +84,46 @@ public static Iterable<Object[]> generateTestParameters() {
{__.filter(__.out().out().count().is(0)),
__.not(__.out().out())},
{__.local(aggregate("x")).count().is(0).as("a"),
__.local(aggregate("x")).limit(1).count().is(0).as("a")},
{__.out().count().as("a").is(0),
__.out().limit(1).count().as("a").is(0)},
- {__.out().count().is(neq(4)),
__.out().limit(5).count().is(neq(4))},
+ {__.out().count().is(neq(4)),
__.out().count().is(neq(4))}, // root: the count value is returned
{__.out().count().is(lte(3)),
__.out().limit(4).count().is(lte(3))},
{__.out().count().is(lt(3)),
__.out().limit(3).count().is(lt(3))},
- {__.out().count().is(gt(2)),
__.out().limit(3).count().is(gt(2))},
- {__.out().count().is(gte(2)),
__.out().limit(2).count().is(gte(2))},
+ {__.out().count().is(gt(2)), __.out().count().is(gt(2))},
// root: the count value is returned
+ {__.out().count().is(gte(2)),
__.out().count().is(gte(2))}, // root: the count value is returned
{__.out().count().is(inside(2, 4)),
__.out().limit(4).count().is(inside(2, 4))},
- {__.out().count().is(outside(2, 4)),
__.out().limit(5).count().is(outside(2, 4))},
+ {__.out().count().is(outside(2, 4)),
__.out().count().is(outside(2, 4))}, // root: the count value is returned
{__.out().count().is(within(2, 6, 4)),
__.out().limit(7).count().is(within(2, 6, 4))},
{__.out().count().is(without(2, 6, 4)),
__.out().limit(6).count().is(without(2, 6, 4))},
+ // a capped count that passes is() must not be observed by
later steps or the parent
+ {__.where(__.out().count().is(gt(1)).is(lt(3))),
__.where(__.out().count().is(gt(1)).is(lt(3)))},
+ {__.where(__.out().count().is(gte(2)).is(lte(2))),
__.where(__.out().count().is(gte(2)).is(lte(2)))},
+ {__.filter(__.out().count().is(neq(1)).is(lt(5))),
__.filter(__.out().count().is(neq(1)).is(lt(5)))},
+ {__.where(__.out().count().is(gt(1)).as("a")),
__.where(__.out().count().is(gt(1)).as("a"))},
+ {__.where(__.out().count().as("a").is(gt(1))),
__.where(__.out().count().as("a").is(gt(1)))},
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
