Cole-Greer commented on code in PR #3717:
URL: https://github.com/apache/tinkerpop/pull/3717#discussion_r4331671550
##########
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:
I think theoretically these cases could be optimized as
```suggestion
{__.where(__.out().count().is(gt(1)).is(lt(3))),
__.where(__.out().count().limit(3).is(gt(1)).is(lt(3)))},
{__.where(__.out().count().is(gte(2)).is(lte(2))),
__.where(__.out().count().limit(3).is(gte(2)).is(lte(2)))},
{__.filter(__.out().count().is(neq(1)).is(lt(5))),
__.filter(__.out().count().limit(5).is(neq(1)).is(lt(5)))},
```
I think the strategy is only looking at the `count().is(gt/gte/neq).<some
other step>` pattern, which misses the context that the full chain of `is()`
steps are all collectively constraining the `count()` in a way which could be
optimized. It's probably ok if we limit the scope of the strategy to the
singular `count().is()` pattern, although I don't think these cases are
necessarily warranted as they could be optimized further.
--
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]
