You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by okram <gi...@git.apache.org> on 2016/05/24 07:24:48 UTC
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1307: NPE with OLTP ne...
GitHub user okram opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/318
TINKERPOP-1307: NPE with OLTP nested group() in an OLAP group() traversal
https://issues.apache.org/jira/browse/TINKERPOP-1307
When an OLAP traversal has a `group()` that contains, nested another `group()`, the latter `group()` is OLTP, but due to transient declarations, a null pointer can arise. To rectify this, `GroupStep.GroupBiOperator` no longer passes around `Traversal` and instead, 4 types: `Traverser`, `TraverserSet`, `Pair<TraverserSet,Object`, `Object`, where `Object` is the current barrier of an internal `Barrier`. Two complex nested group tests were added to demonstrate that things function as expected and there are no longer NPEs.
This change made `group()` a bit slower, but not by much and now the code is safer. For a play test I have, the OLTP speed (which is the important one) went from 436ms to 456ms. Again, no biggie.
CHANGELOG
```
* Fixed a `NullPointerException` bug around nested `group()`-steps in OLAP.
```
`mvn clean install` and integration tests pass.
```
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Apache TinkerPop .................................. SUCCESS [4.643s]
[INFO] Apache TinkerPop :: Gremlin Shaded ................ SUCCESS [2.240s]
[INFO] Apache TinkerPop :: Gremlin Core .................. SUCCESS [36.764s]
[INFO] Apache TinkerPop :: Gremlin Test .................. SUCCESS [11.672s]
[INFO] Apache TinkerPop :: Gremlin Groovy ................ SUCCESS [39.749s]
[INFO] Apache TinkerPop :: Gremlin Groovy Test ........... SUCCESS [7.219s]
[INFO] Apache TinkerPop :: TinkerGraph Gremlin ........... SUCCESS [3:24.052s]
[INFO] Apache TinkerPop :: Gremlin Benchmark ............. SUCCESS [4.156s]
[INFO] Apache TinkerPop :: Hadoop Gremlin ................ SUCCESS [10:46.080s]
[INFO] Apache TinkerPop :: Spark Gremlin ................. SUCCESS [6:40.139s]
[INFO] Apache TinkerPop :: Giraph Gremlin ................ SUCCESS [2:48:03.627s]
[INFO] Apache TinkerPop :: Neo4j Gremlin ................. SUCCESS [1:08:44.849s]
[INFO] Apache TinkerPop :: Gremlin Driver ................ SUCCESS [10.102s]
[INFO] Apache TinkerPop :: Gremlin Server ................ SUCCESS [13:20.898s]
[INFO] Apache TinkerPop :: Gremlin Console ............... SUCCESS [1:24.523s]
[INFO] Apache TinkerPop :: Gremlin Archetype ............. SUCCESS [0.133s]
[INFO] Apache TinkerPop :: Archetype - TinkerGraph ....... SUCCESS [5.907s]
[INFO] Apache TinkerPop :: Archetype - Server ............ SUCCESS [10.589s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 4:34:37.829s
[INFO] Finished at: Mon May 23 22:03:45 MDT 2016
[INFO] Final Memory: 99M/680M
[INFO] ------------------------------------------------------------------------
```
VOTE +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-1307
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/318.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 #318
----
commit c890cebad5231cdab8744878bebb4ea363689a3a
Author: Marko A. Rodriguez <ok...@gmail.com>
Date: 2016-05-23T22:24:58Z
fixed the NPE that occurs in OLAP when you have a local/OLTP group() nested within an OLAP group(). The solution is elegant, but the problem, its not as efficient as the code when we had the NPE.... dar. Going to fiddle some more tomorrow to see if I can get it faster --- 600ms vs 400ms differences.
commit e087123ca9bf513a555b3666a201905df43d8f7c
Author: Marko A. Rodriguez <ok...@gmail.com>
Date: 2016-05-23T23:26:35Z
came up with a nifty trick and now OLD and NEW group() are much closer in time. 432ms vs 456ms. Given that the solution is no longer an NPE in nested groups in OLAP -- this minor time hit is worth it. :). Running integration tests over night.
commit 085be94691357db5615ed4281a978b686bce6898
Author: Marko A. Rodriguez <ok...@gmail.com>
Date: 2016-05-24T07:23:14Z
some code clean up/organization and added a more complex group() nesting that requires an OLAP walk and an OLTP walk. Also found a bunch of HadoopGraph tests that were needlessly being Opt_Out'd -- fixed.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1307: NPE with OLTP ne...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/318#issuecomment-221502808
Full test suite passed, query times look good ...
VOTE: +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1307: NPE with OLTP ne...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/318#issuecomment-221531476
All tests pass with `docker/build.sh -t -n -i`
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1307: NPE with OLTP ne...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/318
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1307: NPE with OLTP ne...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/318#issuecomment-221280324
Did some more benchmarking of `master/` vs. this branch The times are "the same."
```
// result: {cover=368579, =160968, original=771317}
// old vs. new
// STANDARD
Big Values -- by(songType)
group: 432.720949 vs. 440.4774939999999
group: 428.08102429999997 vs. 438.2646092
Big Keys -- by(name)
group: 426.047715 vs. 392.74015999999995
group: 452.2799462 vs. 405.5412201
// COMPUTER
Big Values -- by(songType)
group: 54.3288408 vs. 61.9877934
group: 90.81696539999999 vs. 84.366255
Big Keys -- by(name)
group: 31.852916 vs. 31.7736176
group: 53.63283839999999 vs. 60.662042799999995
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---