fix: Traversal.graph is empty in StepStrategy.apply() with `count().is(0)` by javeme · Pull Request #1699 · apache/tinkerpop

javeme · 2022-06-13T15:39:20Z

This PR fixed:
traversal.getGraph() is empty in StepStrategy.apply() method VertexStepStrategy.apply(Traversal.Admin traversal) for the following Gremlin:

g.V(3).repeat(inE('child').outV().simplePath())
 .until(or(inE().count().is(0),loops().is(eq(2))))
 .path()

Initial data:

g.addV('node').property(id, 1).as('1')
 .addV('node').property(id, 2).as('2')
 .addV('node').property(id, 3).as('3')
 .addV('node').property(id, 4).as('4')
 .addE('child').from('1').to('2')
 .addE('child').from('2').to('3')
 .addE('child').from('4').to('3')

Cause analysis:

In the case of a non count().is(0) statement, the graph context is passed here:

tinkerpop/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java

Line 138 in f8af061

if (hasGraph) t.setGraph(this.graph);

So we can get the graph context when Strategy.apply() is called:

tinkerpop/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java

Line 152 in f8af061

TraversalHelper.applyTraversalRecursively(strategy::apply, this);

But when using the count().is(0) statement, the graph context is missing because the original traversal is replaced with a new traversal by __.start():

tinkerpop/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java

Line 154 in 7f7d3a4

inner = __.start().asAdmin();

The __.start() method implementation:

tinkerpop/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/__.java

Line 59 in 2c24493

public static <A> GraphTraversal<A, A> start() {

javeme · 2022-06-13T15:47:00Z

The following is the stack trace when traversal.getGraph().get() returned null:

2022-06-13 23:18:00 [gremlin-server-exec-2] [WARN] o.a.t.g.s.h.HttpHandlerUtil - Invalid request - responding with 500 Internal Server Error and Error encountered evaluating script: g.V(3).repeat(inE('child').outV().simplePath()).until(or(inE().count().is(0),loops().is(eq(2)))).path()
java.lang.NullPointerException: null
	at com.baidu.hugegraph.traversal.optimize.HugeVertexStepStrategy.apply(HugeVertexStepStrategy.java:73) ~[classes/:?]
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy$TraversalStrategyProxy.apply(HugeGraphAuthProxy.java:1722) ~[classes/:?]
	at org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper.applyTraversalRecursively(TraversalHelper.java:467) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper.applyTraversalRecursively(TraversalHelper.java:475) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper.applyTraversalRecursively(TraversalHelper.java:475) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper.applyTraversalRecursively(TraversalHelper.java:475) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal.applyStrategies(DefaultTraversal.java:151) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal.hasNext(DefaultTraversal.java:221) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils.fill(IteratorUtils.java:62) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils.list(IteratorUtils.java:85) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils.asList(IteratorUtils.java:382) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.server.handler.HttpGremlinEndpointHandler.lambda$channelRead$1(HttpGremlinEndpointHandler.java:221) ~[gremlin-server-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.util.function.FunctionUtils.lambda$wrapFunction$0(FunctionUtils.java:36) ~[gremlin-core-3.5.1.jar:3.5.1]
	at org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor.lambda$eval$0(GremlinExecutor.java:278) ~[gremlin-groovy-3.5.1.jar:3.5.1]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_111]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_111]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_111]
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy$ContextTask.run(HugeGraphAuthProxy.java:1838) [classes/:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [?:1.8.0_111]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [?:1.8.0_111]
	at java.lang.Thread.run(Thread.java:745) [?:1.8.0_111]

spmallette · 2022-06-20T18:23:09Z

sorry no one has had a chance to look at this until now. i think you've identified the problem well. A few things:

you supplied an example in the PR description where this was failing, but could you turn that into an actual Gremlin test?
did you mean to target master? i assume this problem goes all the way back to 3.5.3, right? if so could you please retarget your PR for 3.5-dev branch?
can you please add a CHANGELOG entry?
i'm concerned this is a wider issue within strategies. i just recently fixed a similar issue in PartitionStrategy. i can't help wondering if there is a bigger problem here that needs to be addressed. i dont think strategy authors should have to worry about this detail. i suppose that needs to be looked at separately from this PR though.

javeme · 2022-06-21T08:47:42Z

Thanks for your review.

I will try to add a gremlin test later.
Done.
Done.
Yes, we may need to find a general way to avoid this case, checking one by one is hard work and often miss something.

codecov-commenter · 2022-06-21T09:11:46Z

Codecov Report

Merging #1699 (f7ffd60) into 3.5-dev (0a57213) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           3.5-dev    #1699   +/-   ##
========================================
  Coverage    63.27%   63.27%           
========================================
  Files           23       23           
  Lines         3553     3553           
========================================
  Hits          2248     2248           
  Misses        1131     1131           
  Partials       174      174

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a57213...f7ffd60. Read the comment docs.

spmallette · 2022-06-29T19:58:08Z

i have this more generalized fix i'm considering: 8a7049f

i don't like that it adds yet another round of recursion. i'm questioning if strategy application should even be responsible for this, but its always been this way. i don't think this adds any significant performance penalty as strategy application is already pretty fast. in any event, if you have a way to test this fix on your end, that would be helpful but i'm leaning toward this approach for at least the branches we can't take breaks in.

…s(0)` This PR fix: traversal.getGraph() is empty in StepStrategy.apply() method `VertexStepStrategy.apply(Traversal.Admin<?, ?> traversal)` for the following gremlin: ``` g.V(3).repeat(inE('child').outV().simplePath()) .until(or(inE().count().is(0),loops().is(eq(2)))) .path() ```

javeme · 2022-06-30T06:51:59Z

I think it's ok to assign graph instance after each strategy is applied, it ensures that the apply() of subsequent strategies can get the graph instance.

So do we need to merge this PR as a bug record, or abandon it?

javeme · 2022-06-30T07:40:45Z

I have compiled and tested this fix on hugegraph backend, it works as expected.

spmallette · 2022-06-30T11:12:16Z

if that fix works then i think we would just close this PR and assume the problem satisfied by the change you tested. thanks for bringing this up. i'll close this after the other fix gets merged.

Discussed on #1699 CTR

spmallette · 2022-07-08T10:49:33Z

merged the fix noted above as 1a548e8

spmallette · 2023-04-18T18:44:42Z

@javeme this is a bit old now, but it seems we must revisit it. the fix mentioned here is being rolled back for 3.5.6/3.6.3. that extra recursion is proving too costly. please see the latest work on this here: #2026 if you find the Graph still isn't there for some reason, you can pull it with Traversal.getRoot(traversal).getGraph() or in the worst case, we might want to look into figuring out where else we might get the Graph assigned as part of traversal construction, but so far, five graph implementations seem ok with how this change works.

javeme · 2023-04-19T03:40:24Z

Thank you for your notice.

According to #2026 it seems that the Graph is set in the lock() method, right?
https://github.com/apache/tinkerpop/pull/2026/files#diff-33e6e540ffbb42c4d9ac4a78e83fbb9c28168d0731340b6d1b2e2a680324f081R320

spmallette · 2023-04-19T10:43:49Z

yes - the Graph should get set once now when all strategies are completely applied, but it also means that the Graph might not be set in between strategy applications, so if a strategy needs Graph the only place it can be sure it will find it is in the root traversal.

javeme changed the title ~~fix: Traversal.graph is empty in StepStrategy.apply() with `count().i…~~ fix: Traversal.graph is empty in StepStrategy.apply() with count().is(0) Jun 13, 2022

javeme mentioned this pull request Jun 13, 2022

fix: Traversal.graph is empty in StepStrategy.apply() with count().is(0) apache/hugegraph#1903

Merged

javeme changed the base branch from master to 3.5-dev June 21, 2022 08:21

javeme force-pushed the patch-1 branch from 6dfc2d4 to a8f153c Compare June 21, 2022 08:42

javeme force-pushed the patch-1 branch from b341f70 to f7ffd60 Compare June 21, 2022 08:49

javeme and others added 2 commits June 30, 2022 14:40

add a CHANGELOG entry

0b8dd54

javeme force-pushed the patch-1 branch from f7ffd60 to 0b8dd54 Compare June 30, 2022 06:47

spmallette added a commit that referenced this pull request Jul 8, 2022

Ensure that Graph is set after strategy application

1a548e8

Discussed on #1699 CTR

spmallette closed this Jul 8, 2022

javeme mentioned this pull request Dec 19, 2022

chore: fix TinkerPop unit test apache/hugegraph#2055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Traversal.graph is empty in StepStrategy.apply() with `count().is(0)`#1699

fix: Traversal.graph is empty in StepStrategy.apply() with `count().is(0)`#1699
javeme wants to merge 2 commits intoapache:3.5-devfrom
javeme:patch-1

javeme commented Jun 13, 2022 •

edited

Loading

Uh oh!

javeme commented Jun 13, 2022

Uh oh!

spmallette commented Jun 20, 2022

Uh oh!

javeme commented Jun 21, 2022

Uh oh!

codecov-commenter commented Jun 21, 2022 •

edited

Loading

Uh oh!

spmallette commented Jun 29, 2022

Uh oh!

javeme commented Jun 30, 2022

Uh oh!

javeme commented Jun 30, 2022

Uh oh!

spmallette commented Jun 30, 2022

Uh oh!

spmallette commented Jul 8, 2022

Uh oh!

spmallette commented Apr 18, 2023

Uh oh!

javeme commented Apr 19, 2023

Uh oh!

spmallette commented Apr 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

javeme commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Initial data:

Cause analysis:

Uh oh!

javeme commented Jun 13, 2022

Uh oh!

spmallette commented Jun 20, 2022

Uh oh!

javeme commented Jun 21, 2022

Uh oh!

codecov-commenter commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

spmallette commented Jun 29, 2022

Uh oh!

javeme commented Jun 30, 2022

Uh oh!

javeme commented Jun 30, 2022

Uh oh!

spmallette commented Jun 30, 2022

Uh oh!

spmallette commented Jul 8, 2022

Uh oh!

spmallette commented Apr 18, 2023

Uh oh!

javeme commented Apr 19, 2023

Uh oh!

spmallette commented Apr 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

javeme commented Jun 13, 2022 •

edited

Loading

codecov-commenter commented Jun 21, 2022 •

edited

Loading