Skip to content

Fix CountStrategy returning capped counts when the count is observed #3717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
upadhyay-prashant wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix CountStrategy returning capped counts when the count is observed #3717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))))`.

@Cole-Greer Cole-Greer Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Shorter changelog entries are preferred, perhaps:

Suggested change
* 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))))`.
* 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. `out().count().is(gt(2))`.


[[release-4-0-0-beta-3]]
=== TinkerPop 4.0.0-beta.3 (July 20, 2026)
Expand Down
50 changes: 47 additions & 3 deletions ...a/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,13 @@
import org.apache.tinkerpop.gremlin.process.traversal.step.filter.IsStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.filter.NotStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.map.CountGlobalStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.map.MatchStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.IdentityStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.SideEffectStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.TraversalSideEffectStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.filter.IsStepContract;
import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalStepContract;
import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
Expand All @@ -64,7 +66,8 @@
* @example <pre>
* __.outE().count().is(0) // is replaced by __.not(outE())
* __.outE().count().is(lt(3)) // is replaced by __.outE().limit(3).count().is(lt(3))
* __.outE().count().is(gt(3)) // is replaced by __.outE().limit(4).count().is(gt(3))
* __.where(outE().count().is(gt(3))) // is replaced by __.where(outE().limit(4).count().is(gt(3)))
* __.outE().count().is(gt(3)) // is not changed as the real count is returned when it is greater than 3
* </pre>
*/
public final class CountStrategy extends AbstractTraversalStrategy<TraversalStrategy.OptimizationStrategy> implements TraversalStrategy.OptimizationStrategy {
Expand Down Expand Up @@ -196,7 +199,10 @@ public void apply(final Traversal.Admin<?, ?> traversal) {
}
}
} else {
TraversalHelper.insertBeforeStep(new RangeGlobalStep<>(traversal, 0L, highRange < 0 ? 0 : highRange), curr, traversal);
final long limit = highRange < 0 ? 0 : highRange;
if (isLimitSafe(traversal, isStep, isStepPredicate, limit)) {
TraversalHelper.insertBeforeStep(new RangeGlobalStep<>(traversal, 0L, limit), curr, traversal);
}
}
i++;
}
Expand All @@ -205,6 +211,41 @@ public void apply(final Traversal.Admin<?, ?> traversal) {
}
}

/**
* Determines if a {@code limit(n)} can be placed in front of {@code count().is(predicate)} without changing the
* result. The limit caps the count at {@code n}, which is enough for the {@link IsStep} to decide, but if the
* predicate accepts {@code n} then the capped value passes through in place of the real count. That is only
* acceptable when nothing downstream observes the count value (e.g. a trailing {@code is()} in
* {@code count().is(gt(1)).is(lt(3))}, the root traversal or a {@code map()} child) and the child is only used as
* a filter.
*/
private static boolean isLimitSafe(final Traversal.Admin<?, ?> traversal, final IsStepContract isStep,
final P isStepPredicate, final long limit) {
// if the capped count cannot pass the predicate, any count that does pass is exact
return !isStepPredicate.test(limit) || !isCountValueObserved(traversal, isStep);
}

/**
* Determines if the value emitted by {@code count().is(predicate)} is visible beyond a simple filter decision.
*/
private static boolean isCountValueObserved(final Traversal.Admin<?, ?> traversal, final IsStepContract isStep) {
// a later step can access the count value. this includes the WhereEndStep that where() appends when its
// child ends with a label, as that compares the value to the path. labels elsewhere in a filter child can't
// be read from outside of it, so they don't matter here.
if (!(isStep.getNextStep() instanceof EmptyStep))
return true;

// the repeat() body always ends with a RepeatEndStep so it was already handled above, which leaves until()
// and emit() where the result is only used as a condition
if (traversal.getParent() instanceof RepeatStep) return false;

final TraversalParent parent = traversal.getParent();
// only parents that use the child traversal purely to test for the existence of a result. other FilterStep
// or SideEffectStep parents (e.g. dedup().by() or aggregate().by()) consume the value itself.
return !(parent instanceof TraversalFilterStep || parent instanceof NotStep
|| parent instanceof ConnectiveStep || parent instanceof TraversalSideEffectStep);
}

/**
* The {@code traversal} argument was marked as one that could be converted to {@code not()}, so wrap it inside
* that step and copy the labels.
Expand All @@ -227,6 +268,9 @@ private boolean doStrategy(final Step step) {
final Step parent = step.getTraversal().getParent().asStep();
return (parent instanceof FilterStep || parent.getLabels().isEmpty()) && // if the parent is labeled, then the count matters
!(parent.getNextStep() instanceof MatchStep.MatchEndStep && // if this is in a pattern match, then don't do it.
((MatchStep.MatchEndStep) parent.getNextStep()).getMatchKey().isPresent());
((MatchStep.MatchEndStep) parent.getNextStep()).getMatchKey().isPresent()) &&
// a traversal in the predicate (e.g. is(lt(__.constant(3))) or is(lte(__.select("a")))) is only
// resolved at runtime, so no limit can be derived that is guaranteed not to change the result
!((IsStepContract<?>) step.getNextStep()).getPredicate().hasTraversal();
}
}
58 changes: 53 additions & 5 deletions ...g/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,14 @@ public static Iterable<Object[]> generateTestParameters() {
{__.count().is(1), __.limit(2).count().is(1)},
{__.count().is(-1), __.limit(0).count().is(-1)}, // -1 for RangeGlobalStep means no upper limit
{__.count().is(-2), __.limit(0).count().is(-2)},
// the limit is kept at the root when the capped count can't pass the predicate: a count of 0 fails
// is(-2023) and between() excludes its upper bound of 10, so any count that does pass is exact
{__.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)))},
// limit(11) would cap e.g. 20 at 11, which passes gt(10) and would be returned in place of 20
{__.count().is(P.gt(10).or(lt(5))), __.count().is(P.gt(10).or(lt(5)))},
{__.where(__.count().is(P.gt(10).or(lt(5)))), __.where(__.limit(11).count().is(P.gt(10).or(lt(5))))},
{__.out().count().is(0), __.out().limit(1).count().is(0)},
{__.outE().count().is(lt(1)), __.outE().limit(1).count().is(lt(1))},
{__.both().count().is(lte(0)), __.both().limit(1).count().is(lte(0))},
Expand All @@ -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))},
Comment thread
Cole-Greer marked this conversation as resolved.
{__.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)))},
Comment on lines +106 to +108

@Cole-Greer Cole-Greer Oct 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think theoretically these cases could be optimized as

Suggested change
{__.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)).is(lt(3))), __.where(__.out().limit(3).count().is(gt(1)).is(lt(3)))},
{__.where(__.out().count().is(gte(2)).is(lte(2))), __.where(__.out().limit(3).count().is(gte(2)).is(lte(2)))},
{__.filter(__.out().count().is(neq(1)).is(lt(5))), __.filter(__.out().limit(5).count().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 (the subsequent lt filter drops anything which would've exceeded the limit()). 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.

// a label at the end of the where() child turns it into a WhereTraversalStep, which compares the
// count to the path, so the capped value would be observed
{__.where(__.out().count().is(gt(1)).as("a")), __.where(__.out().count().is(gt(1)).as("a"))},
// other labels in a filter-only child can't be read outside of it, so the limit is safe
{__.where(__.out().count().as("a").is(gt(1))), __.where(__.out().limit(2).count().as("a").is(gt(1)))},
{__.filter(__.out().count().as("a").is(gt(1))), __.filter(__.out().limit(2).count().as("a").is(gt(1)))},
{__.filter(__.out().count().is(gt(1)).as("a")), __.filter(__.out().limit(2).count().is(gt(1)).as("a"))},
{__.repeat(__.out()).until(__.out().count().is(gt(1)).is(lt(3))), __.repeat(__.out()).until(__.out().count().is(gt(1)).is(lt(3)))},
{__.map(__.out().count().is(gt(1))), __.map(__.out().count().is(gt(1)))},
{__.flatMap(__.out().count().is(gte(2))), __.flatMap(__.out().count().is(gte(2)))},
{__.local(__.out().count().is(gt(1))), __.local(__.out().count().is(gt(1)))},
{__.project("a").by(__.out().count().is(gt(1))), __.project("a").by(__.out().count().is(gt(1)))},
{__.dedup().by(__.out().count().is(gt(1))), __.dedup().by(__.out().count().is(gt(1)))},
{__.aggregate("x").by(__.out().count().is(gt(1))), __.aggregate("x").by(__.out().count().is(gt(1)))},
{__.repeat(__.out().count().is(gt(1))).times(1), __.repeat(__.out().count().is(gt(1))).times(1)},
// traversal-valued predicates are resolved at runtime, so no safe limit can be derived, even in
// a filter-only child and even when the traversal reads a label on count()
{__.filter(__.out().count().is(gt(1).and(lt(__.constant(3L))))), __.filter(__.out().count().is(gt(1).and(lt(__.constant(3L)))))},
{__.filter(__.out().count().as("a").is(gt(1).and(lte(__.select("a"))))), __.filter(__.out().count().as("a").is(gt(1).and(lte(__.select("a")))))},
{__.out().count().is(gt(1).and(lte(__.constant(5L)))), __.out().count().is(gt(1).and(lte(__.constant(5L))))},
{__.filter(__.out().count().is(lt(__.constant(1L)))), __.filter(__.out().count().is(lt(__.constant(1L))))},
{__.filter(__.out().count().is(P.not(gt(__.constant(1L))))), __.filter(__.out().count().is(P.not(gt(__.constant(1L)))))},
// still optimized: only exact counts below the limit can pass the first is()
{__.where(__.out().count().is(lt(3)).is(gt(1))), __.where(__.out().limit(3).count().is(lt(3)).is(gt(1)))},
{__.out().count().is(lt(3)).is(gt(1)), __.out().limit(3).count().is(lt(3)).is(gt(1))},
{__.map(__.out().count().is(lte(3))), __.map(__.out().limit(4).count().is(lte(3)))},
// still optimized: the parent only checks whether anything passed
{__.where(__.out().count().is(gt(2))), __.where(__.out().limit(3).count().is(gt(2)))},
{__.not(__.out().count().is(gte(2))), __.not(__.out().limit(2).count().is(gte(2)))},
{__.sideEffect(__.out().count().is(gt(2))), __.sideEffect(__.out().limit(3).count().is(gt(2)))},
{__.repeat(__.out()).until(__.out().count().is(gt(2))), __.repeat(__.out()).until(__.out().limit(3).count().is(gt(2)))},
{__.repeat(__.out()).emit(__.out().count().is(gte(2))), __.repeat(__.out()).emit(__.out().limit(2).count().is(gte(2)))},
{__.out().until(__.out().count().is(gt(2))).repeat(__.out()), __.out().until(__.out().limit(3).count().is(gt(2))).repeat(__.out())},
{__.and(__.out().count().is(gt(1)), __.has("name")), __.and(__.out().limit(2).count().is(gt(1)), __.has("name"))},
{__.or(__.out().count().is(gte(3)), __.has("name")), __.or(__.out().limit(3).count().is(gte(3)), __.has("name"))},
{__.map(__.count().is(0)), __.map(__.limit(1).count().is(0))},
{__.flatMap(__.count().is(0)), __.flatMap(__.limit(1).count().is(0))},
{__.flatMap(__.count().is(0)).as("a"), __.flatMap(__.count().is(0)).as("a")}, // no change
Expand Down
3 changes: 2 additions & 1 deletion ...he/tinkerpop/gremlin/process/traversal/strategy/optimization/LazyBarrierStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ public static Iterable<Object[]> generateTestParameters() {
{__.out().out().out().count(), __.out().out().barrier(LAZY_SIZE).out().count(), Collections.emptyList()},
{__.out().out().out().out().count(), __.out().out().barrier(LAZY_SIZE).out().barrier(LAZY_SIZE).out().count(), Collections.emptyList()},
{__.out().out().out().count(), __.out().out().barrier(LAZY_SIZE).outE().count(), Arrays.asList(CountStrategy.instance(), AdjacentToIncidentStrategy.instance(), GValueReductionStrategy.instance())},
{__.out().out().out().count().is(P.gt(10)), __.out().out().barrier(LAZY_SIZE).outE().limit(11).count().is(P.gt(10)), Arrays.asList(CountStrategy.instance(), AdjacentToIncidentStrategy.instance(), GValueReductionStrategy.instance())},
{__.out().out().out().count().is(P.gt(10)), __.out().out().barrier(LAZY_SIZE).outE().count().is(P.gt(10)), Arrays.asList(CountStrategy.instance(), AdjacentToIncidentStrategy.instance(), GValueReductionStrategy.instance())},
{__.where(__.out().out().out().count().is(P.gt(10))), __.where(__.out().out().barrier(LAZY_SIZE).outE().limit(11).count().is(P.gt(10))), Arrays.asList(CountStrategy.instance(), AdjacentToIncidentStrategy.instance(), GValueReductionStrategy.instance())},
{__.outE().inV().outE().inV().outE().inV().groupCount(), __.outE().inV().outE().inV().barrier(LAZY_SIZE).outE().inV().groupCount(), Collections.emptyList()},
{__.outE().inV().outE().inV().outE().inV().groupCount(), __.out().out().barrier(LAZY_SIZE).out().groupCount(), List.of(IncidentToAdjacentStrategy.instance(), GValueReductionStrategy.instance())},
{__.out().out().has("age", 32).out().count(), __.out().out().barrier(LAZY_SIZE).has("age", 32).out().count(), Collections.emptyList()},
Expand Down
4 changes: 3 additions & 1 deletion ...st/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalExplanationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ public void shouldApplyStrategiesCorrectly() {
traversal.setStrategies(TraversalStrategies.GlobalCache.getStrategies(Graph.class).clone());
checkTraversalExplanation(traversal.explain(), List.of(new ExplainExpectation(AdjacentToIncidentStrategy.class, "[VertexStepPlaceholder(IN,edge)")));
///
traversal = __.outE().inV().group().by(__.inE().outV().groupCount().by(__.both().count().is(P.gt(2)))).asAdmin();
// lt(3) so that CountStrategy can safely limit: the count is a by() key, so a predicate that accepts the cap
// (e.g. gt(2)) must be left unlimited
traversal = __.outE().inV().group().by(__.inE().outV().groupCount().by(__.both().count().is(P.lt(3)))).asAdmin();
traversal.setStrategies(TraversalStrategies.GlobalCache.getStrategies(Graph.class).clone().removeStrategies(ProductiveByStrategy.class));
checkTraversalExplanation(traversal.explain(), List.of(new ExplainExpectation(IncidentToAdjacentStrategy.class, "[VertexStepPlaceholder(IN,vertex)"),
new ExplainExpectation(IncidentToAdjacentStrategy.class, "[VertexStepPlaceholder(OUT,vertex)"),
Expand Down
Loading
Loading