Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 3.0.2-incubating, 3.1.3, 3.2.1
    • 3.1.4, 3.2.2, 3.3.0
    • process
    • None

    Description

      https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/TailGlobalStep.java#L71-L74

      This code doesn't account for the excess removed from tailBulk. This can cause the code to set incorrect bulk values when there are multiple traversers in the tail buffer.

      I observed this behavior intermittently in TitanGraph (https://github.com/thinkaurelius/titan/pull/1312), which doesn't allow user defined ids, so the ordering of the traversers in the TraverserSet is a lot more random than the unit tests using TinkerGraph.

      This issue is reproducible with TinkerGraph (master/3.2.1). Instead of loading from one of the data files, manually create the graph with the ids in inverted order.

      graph = TinkerGraph.open();
      
      // graph.io(IoCore.gryo()).readGraph("tinkerpop-modern.kryo");
      final Vertex v1 = graph.addVertex(T.id, 6L, T.label, "person", "name", "marko", "age", 29);
      final Vertex v2 = graph.addVertex(T.id, 5L, T.label, "person", "name", "vadas", "age", 27);
      final Vertex v3 = graph.addVertex(T.id, 4L, T.label, "software", "name", "lop", "lang", "java");
      final Vertex v4 = graph.addVertex(T.id, 3L, T.label, "person", "name", "josh", "age", 32);
      final Vertex v5 = graph.addVertex(T.id, 2L, T.label, "software", "name", "ripple", "lang", "java");
      final Vertex v6 = graph.addVertex(T.id, 1L, T.label, "person", "name", "peter", "age", 35);
      v1.addEdge("knows", v2, "weight", 0.5d);
      v1.addEdge("knows", v4, "weight", 1.0d);
      v1.addEdge("created", v2, "weight", 0.4d);
      v4.addEdge("knows", v5, "weight", 1.0d);
      v4.addEdge("knows", v3, "weight", 0.4d);
      v6.addEdge("knows", v3, "weight", 0.2d);
      if (graph.features().graph().supportsTransactions()) graph.tx().commit();
      
      final GraphTraversalSource g = graph.traversal();
      String result = g.V().repeat(both()).times(3).tail(7).count().next().toString();
      boolean success = "7".equals(result);
      

      The fix is this:

      if (excess > 0) {
          oldest.setBulk(oldestBulk-excess);
          // Account for the loss of excess in the tail buffer
          this.tailBulk -= excess;
      }
      

      Attachments

        Issue Links

          Activity

            Great. If you have a fix, please provide a PR that also has an added test case to TailTest so we know things are solid.

            okram Marko A. Rodriguez added a comment - Great. If you have a fix, please provide a PR that also has an added test case to TailTest so we know things are solid.
            pietermartin pieter martin added a comment -

            This is causing Sqlg's tests to fail on 3.2.1.
            I tested with your proposed fix and it fixes the issue.

            pietermartin pieter martin added a comment - This is causing Sqlg's tests to fail on 3.2.1. I tested with your proposed fix and it fixes the issue.
            githubbot ASF GitHub Bot added a comment -

            GitHub user pluradj opened a pull request:

            https://github.com/apache/tinkerpop/pull/363

            TINKERPOP-1379 remove excess bulk in tail buffer

            https://issues.apache.org/jira/browse/TINKERPOP-1379

            You can merge this pull request into a Git repository by running:

            $ git pull https://github.com/apache/tinkerpop TINKERPOP-1379

            Alternatively you can review and apply these changes as the patch at:

            https://github.com/apache/tinkerpop/pull/363.patch

            To close this pull request, make a commit to your master/trunk branch
            with (at least) the following in the commit message:

            This closes #363


            commit d43db2bb2f3f4988503f0cdedbd1877cd542c502
            Author: Jason Plurad <pluradj@us.ibm.com>
            Date: 2016-07-20T16:31:53Z

            remove excess bulk in tail buffer


            githubbot ASF GitHub Bot added a comment - GitHub user pluradj opened a pull request: https://github.com/apache/tinkerpop/pull/363 TINKERPOP-1379 remove excess bulk in tail buffer https://issues.apache.org/jira/browse/TINKERPOP-1379 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1379 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/363.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #363 commit d43db2bb2f3f4988503f0cdedbd1877cd542c502 Author: Jason Plurad <pluradj@us.ibm.com> Date: 2016-07-20T16:31:53Z remove excess bulk in tail buffer
            githubbot ASF GitHub Bot added a comment -

            Github user pluradj commented on the issue:

            https://github.com/apache/tinkerpop/pull/363

            I've come up with a cleaner test case. I'll close this PR and open a new one.

            githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the issue: https://github.com/apache/tinkerpop/pull/363 I've come up with a cleaner test case. I'll close this PR and open a new one.
            githubbot ASF GitHub Bot added a comment -

            Github user pluradj closed the pull request at:

            https://github.com/apache/tinkerpop/pull/363

            githubbot ASF GitHub Bot added a comment - Github user pluradj closed the pull request at: https://github.com/apache/tinkerpop/pull/363
            githubbot ASF GitHub Bot added a comment -

            GitHub user pluradj opened a pull request:

            https://github.com/apache/tinkerpop/pull/366

            TINKERPOP-1379 remove excess bulk in tail buffer

            https://issues.apache.org/jira/browse/TINKERPOP-1379

            VOTE: +1

            You can merge this pull request into a Git repository by running:

            $ git pull https://github.com/apache/tinkerpop TINKERPOP-1379

            Alternatively you can review and apply these changes as the patch at:

            https://github.com/apache/tinkerpop/pull/366.patch

            To close this pull request, make a commit to your master/trunk branch
            with (at least) the following in the commit message:

            This closes #366


            commit db7526f477579ee7e731e51749c49e6de2c1e964
            Author: Jason Plurad <pluradj@us.ibm.com>
            Date: 2016-07-30T04:04:54Z

            remove excess bulk in tail buffer


            githubbot ASF GitHub Bot added a comment - GitHub user pluradj opened a pull request: https://github.com/apache/tinkerpop/pull/366 TINKERPOP-1379 remove excess bulk in tail buffer https://issues.apache.org/jira/browse/TINKERPOP-1379 VOTE: +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1379 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/366.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #366 commit db7526f477579ee7e731e51749c49e6de2c1e964 Author: Jason Plurad <pluradj@us.ibm.com> Date: 2016-07-30T04:04:54Z remove excess bulk in tail buffer
            githubbot ASF GitHub Bot added a comment -

            Github user pluradj commented on the issue:

            https://github.com/apache/tinkerpop/pull/366

            Test case `g.V().repeat(__.in().out()).times(3).tail(7).count()` against the modern graph returns `-62`. Fails all the way back to 3.0.x.

            githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the issue: https://github.com/apache/tinkerpop/pull/366 Test case `g.V().repeat(__.in().out()).times(3).tail(7).count()` against the modern graph returns `-62`. Fails all the way back to 3.0.x.
            githubbot ASF GitHub Bot added a comment -

            Github user okram commented on a diff in the pull request:

            https://github.com/apache/tinkerpop/pull/366#discussion_r73580040

            — Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/TailTest.java —
            @@ -120,6 +123,19 @@ public void g_V_repeatXbothX_timesX3X_tailX7X()

            { assertEquals(7, counter); }

            + /** Scenario: Global scope, using repeat (excess BULK) */
            + @Test
            + @LoadGraphWith(MODERN)
            + public void g_V_repeatXin_outX_timesX3X_tailX7X_count() {
            + final Traversal<Vertex, Long> traversal = get_g_V_repeatXin_outX_timesX3X_tailX7X_count();
            + printTraversalForm(traversal);
            + long count = 0L;
            — End diff –

            This is as easy as:

            ```
            checkResults(Collections.singleList(7L), traversal)
            ```

            It will also verify `hasNext() == false` at the end.

            githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/366#discussion_r73580040 — Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/TailTest.java — @@ -120,6 +123,19 @@ public void g_V_repeatXbothX_timesX3X_tailX7X() { assertEquals(7, counter); } + /** Scenario: Global scope, using repeat (excess BULK) */ + @Test + @LoadGraphWith(MODERN) + public void g_V_repeatXin_outX_timesX3X_tailX7X_count() { + final Traversal<Vertex, Long> traversal = get_g_V_repeatXin_outX_timesX3X_tailX7X_count(); + printTraversalForm(traversal); + long count = 0L; — End diff – This is as easy as: ``` checkResults(Collections.singleList(7L), traversal) ``` It will also verify `hasNext() == false` at the end.
            githubbot ASF GitHub Bot added a comment -

            Github user okram commented on the issue:

            https://github.com/apache/tinkerpop/pull/366

            VOTE +1.

            Note that I added a comment for how to make the test case simpler.

            githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/366 VOTE +1. Note that I added a comment for how to make the test case simpler.
            githubbot ASF GitHub Bot added a comment -

            Github user spmallette commented on the issue:

            https://github.com/apache/tinkerpop/pull/366

            All good with a quick `mvn clean install`

            VOTE +1 - pending fix @okram suggested for the test code and what looks like the need for a rebase given conflicts that are likely on changelog.

            githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/366 All good with a quick `mvn clean install` VOTE +1 - pending fix @okram suggested for the test code and what looks like the need for a rebase given conflicts that are likely on changelog.
            githubbot ASF GitHub Bot added a comment -

            Github user asfgit closed the pull request at:

            https://github.com/apache/tinkerpop/pull/366

            githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/366

            People

              pluradj Jason Plurad
              pluradj Jason Plurad
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: