----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54680/#review163667 -----------------------------------------------------------
Fix it, then Ship it! Fix it then ship it common/src/main/java/org/apache/atlas/groovy/ClosureExpression.java (line 35) <https://reviews.apache.org/r/54680/#comment235141> Consider renaming to VariableDeclaration, no need to abbreviate the name. common/src/main/java/org/apache/atlas/groovy/CodeBlockExpression.java (line 25) <https://reviews.apache.org/r/54680/#comment235140> typo repository/src/main/java/org/apache/atlas/gremlin/Gremlin2ExpressionFactory.java (line 185) <https://reviews.apache.org/r/54680/#comment235155> remove commented-out code repository/src/main/java/org/apache/atlas/gremlin/Gremlin3ExpressionFactory.java (lines 291 - 292) <https://reviews.apache.org/r/54680/#comment235157> Rename to startIndex and endIndex repository/src/main/java/org/apache/atlas/gremlin/optimizer/ExpandOrsOptimization.java (line 43) <https://reviews.apache.org/r/54680/#comment235158> Is it Gremlin that does not use the indices, or is it Titan? I thought that was a Titan bug. repository/src/main/java/org/apache/atlas/gremlin/optimizer/GremlinQueryOptimizer.java (lines 34 - 35) <https://reviews.apache.org/r/54680/#comment235161> It's the GremlinOptimization implementations that apply the specific optimizations. This javadoc implies that GremlinQueryOptimizer is doing it. Perhaps it makes sense to have the details of each optimization documented in the implementation classes, and change this javadoc to be more general. repository/src/main/java/org/apache/atlas/gremlin/optimizer/OptimizationContext.java (lines 40 - 42) <https://reviews.apache.org/r/54680/#comment235162> Remove empty comment repository/src/main/java/org/apache/atlas/gremlin/optimizer/PathExpressionFinder.java (line 25) <https://reviews.apache.org/r/54680/#comment235163> Incorrect javadoc, should mention path expressions not aliases. repository/src/main/java/org/apache/atlas/util/AtlasRepositoryConfiguration.java (lines 157 - 160) <https://reviews.apache.org/r/54680/#comment235166> I think is comment is obsolete - a holdover from the original superTypes optimization - and should be removed. - David Kantor On Jan. 31, 2017, 2:11 p.m., Jeff Hagelberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54680/ > ----------------------------------------------------------- > > (Updated Jan. 31, 2017, 2:11 p.m.) > > > Review request for atlas and David Kantor. > > > Bugs: ATLAS-1369 > https://issues.apache.org/jira/browse/ATLAS-1369 > > > Repository: atlas > > > Description > ------- > > The way the queries are generated now, in many cases the indices are not used > when executing queries. The main problem is that gremlin cannot currently use > "has" expressions within an "and" or an "or". This JIRA introduces a > GremlinQueryOptimizer that attempts to extract "has" expressions out of "and" > expressions and turn "or" expressions into unions. > > > Major changes: > > * Updated version of Guava used by Atlas, so that could use > Lists.cartesianProduct > * New package - org.apache.atlas.gremlin.optimizer - contains > optimization framework > * Refactoring of Groovy Expression classes > * new - AbstractFunctionExpression - super class of anything with a > caller > * added 4 new methods to GroovyExpression > * now, each GroovyExpression is a TraversalStepType associated with it > These are used by the optimization framework > > * Added feature toggle : atlas.query.gremlinOptimizerEnabled. The feature > turns optimization on/off. It is provided as a fallback in the event that > issues are found with the optimizer. > > * Most of the new logic is in GremlinQueryOptimizer, > ExpandAndsOptimization, and ExpandOrsOptimization > * > Note - with these changes we saw a very significant improvement when using > IBM Graph as the Atlas Backend (which uses Titan 1 under the covers). > > Please review these changes. Silence implies consent. > > > Diffs > ----- > > > common/src/main/java/org/apache/atlas/groovy/AbstractFunctionExpression.java > PRE-CREATION > common/src/main/java/org/apache/atlas/groovy/AbstractGroovyExpression.java > 49eaae87e593d4e350ecb9a51a83e967d95b514b > common/src/main/java/org/apache/atlas/groovy/ArithmeticExpression.java > 0aec5d03ea6e2a108129b20003d390e826f76bb5 > common/src/main/java/org/apache/atlas/groovy/BinaryExpression.java > ccc9204cdc9613efeb2b6548d8d426bee7cfec06 > common/src/main/java/org/apache/atlas/groovy/CastExpression.java > 963724c7cefcd871918d340c25c197143d155d56 > common/src/main/java/org/apache/atlas/groovy/ClosureExpression.java > 2d702093937b69a7d82e8d5f58fb528144377de5 > common/src/main/java/org/apache/atlas/groovy/CodeBlockExpression.java > 9a726f2f82764b0a93c523467b3fa316e2dbf4e0 > common/src/main/java/org/apache/atlas/groovy/ComparisonExpression.java > 345f838635f62457b0fd42efed83fd5b96b5c16c > > common/src/main/java/org/apache/atlas/groovy/ComparisonOperatorExpression.java > 63638b73b048ed301f5a172bf7cec0e2f618d019 > common/src/main/java/org/apache/atlas/groovy/FieldExpression.java > f6d06bd194e91d332fd7418da2a9ff24342b4647 > common/src/main/java/org/apache/atlas/groovy/FunctionCallExpression.java > dd9b1d58e944a4c172a571f4dcacc6601bce22be > common/src/main/java/org/apache/atlas/groovy/GroovyExpression.java > 493bd3df1274fe67f47abe6606a2f447af8fa470 > common/src/main/java/org/apache/atlas/groovy/IdentifierExpression.java > 6abdbf02ed54389f6bd7d9516fa14dad5d791b8d > common/src/main/java/org/apache/atlas/groovy/ListExpression.java > f7acaace9c4e5c9f87c596f7c11c4c41c1b13db5 > common/src/main/java/org/apache/atlas/groovy/LiteralExpression.java > 008c885686985ff38e0d1ad28a5e0f736e700a39 > common/src/main/java/org/apache/atlas/groovy/LogicalExpression.java > ee5829b39e8c4bd6c80942328100259d2762c506 > common/src/main/java/org/apache/atlas/groovy/RangeExpression.java > 7322f697474f05630398c6379ec0aa4c3ba178d6 > common/src/main/java/org/apache/atlas/groovy/StatementListExpression.java > PRE-CREATION > common/src/main/java/org/apache/atlas/groovy/TernaryOperatorExpression.java > 75a2f8682f62d1d7bf173a6d704c7accb0c27808 > common/src/main/java/org/apache/atlas/groovy/TraversalStepType.java > PRE-CREATION > common/src/main/java/org/apache/atlas/groovy/TypeCoersionExpression.java > 4a6105234fe9ca33830516558a40f459ad7082f9 > > common/src/main/java/org/apache/atlas/groovy/VariableAssignmentExpression.java > 7e018f1dcffed654b958c59d70378d6178db4367 > distro/src/conf/atlas-application.properties > d9e2f6efae611b1360d1a14ecabc981247f2a5b8 > graphdb/titan0/pom.xml 58a5cb852ad9d1e78fc8ced0b153c39d7d816132 > intg/pom.xml 52b5ef544c5ad5f97ef78fd2c8eb979f5e2a7b6d > pom.xml a98579276b7d941e431b35594e363e07fee4efc0 > > repository/src/main/java/org/apache/atlas/gremlin/Gremlin2ExpressionFactory.java > 18587391dc8d384d29e069e04a3bf787d5865452 > > repository/src/main/java/org/apache/atlas/gremlin/Gremlin3ExpressionFactory.java > e862769b7674449a2bebc8f35fdfa07e310eeb88 > > repository/src/main/java/org/apache/atlas/gremlin/GremlinExpressionFactory.java > 6c326b2ea9310fde9515dd98afbb8f97f73b8888 > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/AliasFinder.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/CallHierarchyVisitor.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/ExpandAndsOptimization.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/ExpandOrsOptimization.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/ExpressionFinder.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/FunctionGenerator.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/GremlinOptimization.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/GremlinQueryOptimizer.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/HasForbiddenType.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/gremlin/optimizer/IsOr.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/gremlin/optimizer/IsOrParent.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/OptimizationContext.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/OrderFinder.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/PathExpressionFinder.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/RangeFinder.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/SplitPointFinder.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/gremlin/optimizer/UpdatedExpressions.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedMetadataRepository.java > be02891cad3020e50d5244fd2aaf3c4af3a5954f > > repository/src/main/java/org/apache/atlas/repository/graph/GraphToTypedInstanceMapper.java > 7b2b753b930467e015d364819c5baa983fce0cf8 > > repository/src/main/java/org/apache/atlas/util/AtlasRepositoryConfiguration.java > a04dd95d01d6faa4beeadb042553ba2028a9f1eb > repository/src/main/scala/org/apache/atlas/query/GremlinQuery.scala > f7ba71a444ed327a03c061520646c868c0ba4814 > > repository/src/test/java/org/apache/atlas/discovery/GraphBackedDiscoveryServiceTest.java > f2ca6a8fb1573755d16497236358358a07881e9c > > repository/src/test/java/org/apache/atlas/repository/graph/AbstractGremlinQueryOptimizerTest.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/repository/graph/Gremlin2QueryOptimizerTest.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/repository/graph/Gremlin3QueryOptimizerTest.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/repository/graph/TestIntSequence.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/util/CompiledQueryCacheKeyTest.java > c926f4d7d793ef095b20853277f3ba979483e039 > repository/src/test/scala/org/apache/atlas/query/GremlinTest.scala > 22717ff7433958142dbf126a9bd6e72bc9151d12 > > Diff: https://reviews.apache.org/r/54680/diff/ > > > Testing > ------- > > Ran full Atlas build, no regresssions found. > > > Thanks, > > Jeff Hagelberg > >
