upadhyay-prashant commented on code in PR #3717:
URL: https://github.com/apache/tinkerpop/pull/3717#discussion_r4340381869
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java:
##########
@@ -84,15 +88,59 @@ 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
+ {__.where(__.out().count().is(neq(4))),
__.where(__.out().limit(5).count().is(neq(4)))},
+ {__.out().where(__.count().is(neq(4))),
__.out().where(__.limit(5).count().is(neq(4)))},
{__.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
+ {__.filter(__.out().count().is(gt(2))),
__.filter(__.out().limit(3).count().is(gt(2)))},
+ {__.out().count().is(gte(2)),
__.out().count().is(gte(2))}, // root: the count value is returned
+ {__.filter(__.out().count().is(gte(2))),
__.filter(__.out().limit(2).count().is(gte(2)))},
{__.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
+ {__.where(__.out().count().is(outside(2, 4))),
__.where(__.out().limit(5).count().is(outside(2, 4)))},
{__.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)))},
Review Comment:
Good call, implemented. For a chain of `is()` steps the limit is now the
largest limit needed by any predicate in the chain. Each predicate gives the
same result for every count at or above its own limit, so with that cap the
capped count passes the whole chain exactly when the real count does. The limit
is then added if the chain rejects the cap (any count that passes is exact, so
this works at the root too), or if the count is only used as a filter. Your
three cases now get `limit(3)`, `limit(3)` and `limit(5)`. There are also tests
for chains that accept the cap (`where(...is(gt(1)).is(neq(2)))` gets
`limit(3)`, but the root form is left alone) and for a traversal-valued
predicate later in the chain, which disables it.
Working through this turned up two existing off-by-one limits, both fixed
here with unit tests and feature scenarios:
- `without()` needs max + 1, not max. `without(2, 6, 4)` used `limit(6)`, so
a real count of 7 was capped to 6 and rejected.
- `not(lte(n))` is `gt(n)` and needs n + 1. It used `limit(n)`, which
rejected every count.
##########
CHANGELOG.asciidoc:
##########
@@ -29,6 +29,7 @@
image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Fixed `gremlin-go` to report a malformed or truncated GraphBinary response
as a deserialization error rather than a bare decoder message.
* Made `TinkerGraph` an interface and renamed the in-memory implementation to
`TinkerMemoryGraph`; `TinkerGraph.open()` and `gremlin.graph=...TinkerGraph`
behave as before. *(breaking)*
* Renamed `TinkerTransactionGraph` to `TinkerStorageGraph`. *(breaking)*
+* Fixed `CountStrategy` so that it no longer adds a `limit()` before
`count().is(P)` when the capped count can pass the predicate and is then
observed downstream, e.g. `where(out().count().is(gt(1)).is(lt(3)))` or
`out().count().is(gt(2))`, or when `P` contains a traversal, e.g.
`is(gt(1).and(lt(__.constant(3))))`.
Review Comment:
Done, I used your wording.
--
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]
