Re: CountStrategy and TraversalHelper.replaceStep

[pieter gmail]

Ok great, I'll test a bit with the order changed.

Thanks,
Pieter

On 30/10/2017 18:01, Daniel Kuppitz wrote:

Ah, yea, I see what you mean. It's actually replaceStep() which is buggy,
not the strategy. The fix is easy, we just need to change the order of the
2 statements in replaceStep():

public static <S, E> void replaceStep(final Step<S, E> removeStep,
final Step<S, E> insertStep, final Traversal.Admin<?, ?> traversal) {
     final int i;    traversal.removeStep(i = stepIndex(removeStep,
traversal));    traversal.addStep(i, insertStep);}


Cheers,
Daniel

On Mon, Oct 30, 2017 at 7:43 AM, pieter gmail <pieter.mar...@gmail.com>
wrote:

I am on the latest 3.3.1-SNAPSHOT, just pulled master again.

The actual traversals and results are correct. However after the
CountStrategy the VertexStep(OUT,edge) previousStep pointer
(AbstractStep.previousStep) is incorrect. I should be EmptyStep but infact
points to the newly inserted NotStep.

toString() on a traversal does not show the previous/nextStep so you can
not see what I am talking about in the console. This is not breaking
anything in TinkerGraph but breaks stuff in Sqlg.

In CountStrategy if I change line 153 to then my problems go away and
TinkerGraph also works as expected.

//TraversalHelper.replaceStep(prev, new NotStep<>(traversal, inner),
traversal);
NotStep notStep = new NotStep<>(traversal, inner);
TraversalHelper.replaceStep(prev, notStep, traversal);
List<Traversal.Admin<?, ?>> notStepTraversal = notStep.getLocalChildren();
Traversal.Admin<?, ?> notStepTraversalFirstStep = notStepTraversal.get(0);
//The first step is pointing to the NotStep, it should point to an
EmptyStep
notStepTraversalFirstStep.getSteps().get(0).setPreviousStep(
EmptyStep.instance());

So I suppose the question is, is it correct for the previousStep of the
first step of a local traversal to point to the parent step and not an
EmptyStep?

The TraversalHelper.replaceStep always makes the first step of the
traversal point to the newly inserted step. If the traversal however is a
local traversal then the root step should be an EmptyStep.

Hope it makes some sense.

Thanks
Pieter



On 30/10/2017 15:28, Daniel Kuppitz wrote:

I don't see any issues. Which version are you talking about?

*gremlin> Gremlin.version()*
*==>3.2.7-SNAPSHOT*
gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
gremlin> g.V(1).repeat(out()).until(__.not(outE())).values('name')
==>lop
==>vadas
==>ripple
==>lop
gremlin> g.V(1).repeat(out()).until(outE().count().is(0)).values('name')
==>lop
==>vadas
==>ripple
==>lop
gremlin>
g.V(1).repeat(out()).until(__.not(outE())).values('name').it
erate().toString()
==>[TinkerGraphStep(vertex,[1]), RepeatStep([VertexStep(OUT,vertex),
RepeatEndStep],until([NotStep([VertexStep(OUT,edge)])]),emit(false)),
PropertiesStep([name],value)]
gremlin>
g.V(1).repeat(out()).until(outE().count().is(0)).values('nam
e').iterate().toString()
==>[TinkerGraphStep(vertex,[1]), RepeatStep([VertexStep(OUT,vertex),
RepeatEndStep],until([NotStep([VertexStep(OUT,edge)])]),emit(false)),
PropertiesStep([name],value)]


*gremlin> Gremlin.version()*
*==>3.3.1-SNAPSHOT*

gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
gremlin> g.V(1).repeat(out()).until(__.not(outE())).values('name')
==>lop
==>vadas
==>ripple
==>lop
gremlin> g.V(1).repeat(out()).until(outE().count().is(0)).values('name')
==>lop
==>vadas
==>ripple
==>lop
gremlin>
g.V(1).repeat(out()).until(__.not(outE())).values('name').it
erate().toString()
==>[TinkerGraphStep(vertex,[1]), RepeatStep([VertexStep(OUT,vertex),
RepeatEndStep],until([NotStep([VertexStep(OUT,edge)])]),emit(false)),
PropertiesStep([name],value)]
gremlin>
g.V(1).repeat(out()).until(outE().count().is(0)).values('nam
e').iterate().toString()
==>[TinkerGraphStep(vertex,[1]), RepeatStep([VertexStep(OUT,vertex),
RepeatEndStep],until([NotStep([VertexStep(OUT,edge)])]),emit(false)),
PropertiesStep([name],value)]


Cheers,
Daniel


On Mon, Oct 30, 2017 at 1:53 AM, pieter gmail <pieter.mar...@gmail.com>
wrote:

Hi,

Whilst optimizing the NotStep I came across what looks to me like a bug
in
TraversalHelper.replaceStep or perhaps rather in CountStrategy.

The test below shows the bug.

      @Test
      public void g_VX1X_repeatXoutX_untilXoutE_count_isX0XX_name() {
          Graph graph = TinkerFactory.createModern();
          final Traversal<Vertex, String> traversal1 = graph.traversal()
                  .V(convertToVertexId(graph, "marko"))
                  .repeat(__.out())
                  .until(__.not(__.outE()))
                  .values("name");
          checkResults(Arrays.asList("lop", "lop", "ripple", "vadas"),
traversal1);

          List<VertexStep> vertexSteps = TraversalHelper.getStepsOfAssi
gnableClassRecursively(VertexStep.class, traversal1.asAdmin());
          VertexStep vertexStep = vertexSteps.stream().filter(s ->
s.returnsEdge()).findAny().get();
          Assert.assertEquals(EmptyStep.instance(),
vertexStep.getPreviousStep());

          final Traversal<Vertex, String> traversal2 = graph.traversal()
                  .V(convertToVertexId(graph, "marko"))
                  .repeat(__.out())
                  .until(__.outE().count().is(0))
                  .values("name");
          checkResults(Arrays.asList("lop", "lop", "ripple", "vadas"),
traversal2);

          vertexSteps = TraversalHelper.getStepsOfAssi
gnableClassRecursively(VertexStep.class, traversal2.asAdmin());
          vertexStep = vertexSteps.stream().filter(s ->
s.returnsEdge()).findAny().get();

          //This fails because the vertexStep's previous step is the
NotStepwhen it should be an EmptyStep.
          Assert.assertEquals(EmptyStep.instance(),
vertexStep.getPreviousStep());
      }

traversal1 and traversal2 should be the same as the CountStrategy will
replace the __.outE().count().is(0) with __.not(__.outE())
The CountStrategy does what its suppose to do however then it calls
TraversalHelper.replaceStep(prev, new NotStep<>(traversal, inner),
traversal); the traversal's VertexStep gets its previousStep set to the
NotStep. This is because of the way TraversalHelper.replaceStep is
implemented.

I am not sure whether the fix should be in replaceStep or rather in
CountStrategy.

Thanks
Pieter