----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73128/#review222470 -----------------------------------------------------------
repository/src/main/java/org/apache/atlas/query/SelectClauseComposer.java Lines 231 (patched) <https://reviews.apache.org/r/73128/#comment311564> Shouldn't the argument to setIsPrimitiveAttr() be index into items (and not primitiveTypeCount)? repository/src/main/java/org/apache/atlas/query/executors/GremlinClauseToTraversalTranslator.java Lines 52 (patched) <https://reviews.apache.org/r/73128/#comment311551> subTraversals is used only within process(). Consider moving this field as a local variable within process(). repository/src/main/java/org/apache/atlas/query/executors/ScriptEngineBasedExecutor.java Lines 40 (patched) <https://reviews.apache.org/r/73128/#comment311552> Please consider marking all fields as final. repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 145 (patched) <https://reviews.apache.org/r/73128/#comment311555> if vertex doesn't have the property, would value be null? Please review and update to handle this case. repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 152 (patched) <https://reviews.apache.org/r/73128/#comment311562> maxV is used only within 'if' block at #156. Consider moving #152 to inside 'if' block. repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 154 (patched) <https://reviews.apache.org/r/73128/#comment311560> final? repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 160 (patched) <https://reviews.apache.org/r/73128/#comment311559> AtlasNumericTypeComparator handles string values as well. Is this else block better than using AtlasNumericTypeComparator - as in if block? repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 168 (patched) <https://reviews.apache.org/r/73128/#comment311563> maxV is used only within 'if' block at #172. Consider moving #168 to inside 'if' block. repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 170 (patched) <https://reviews.apache.org/r/73128/#comment311561> final? repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 176 (patched) <https://reviews.apache.org/r/73128/#comment311558> AtlasNumericTypeComparator handles string values as well. Is this else block better than using AtlasNumericTypeComparator - as in if block? repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 185 (patched) <https://reviews.apache.org/r/73128/#comment311557> AtlasNumericTypeComparator handles String values as well. What type of values this class doesn't handle? Perhaps this should be renamed as AtlasAttributeValueComparator? repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java Lines 198 (patched) <https://reviews.apache.org/r/73128/#comment311556> Consider handling null values: if (p1 == null && p2 == null) { return 0; } else if (p1 == null) { return -1; } else if (p2 == null) { return 1; } else if (p1 instanceof String && p2 instanceof String) { ... repository/src/main/java/org/apache/atlas/query/executors/TraversalBasedExecutor.java Lines 72 (patched) <https://reviews.apache.org/r/73128/#comment311553> getAtlasSearchResult() => getSearchResult() repository/src/main/java/org/apache/atlas/query/executors/TraversalBasedExecutor.java Lines 73 (patched) <https://reviews.apache.org/r/73128/#comment311554> resultList will not be empty, ref #67; Please review and remove #73 - #76. - Madhan Neethiraj On Jan. 14, 2021, 1:04 a.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/73128/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2021, 1:04 a.m.) > > > Review request for atlas, Apoorv Naik, Nikhil Bonte, Nixon Rodrigues, Pinal > Shah, and Sarath Subramanian. > > > Bugs: ATLAS-2932 > https://issues.apache.org/jira/browse/ATLAS-2932 > > > Repository: atlas > > > Description > ------- > > **Approach** > General Approach: > - Ensure that Gremlin-based implementation continues to function. This will > ensure backward compatibility and will guarad against regressions or behavior > deviations, if any, are introduced with the new implementation. > - Continue using existing _GremlinQueryComposer_. > - Translate the clauses to _AtlasGraphTraversal_. > - Projects that were earlier handled in Gremlin query are now handled in a > separate Java class. > > Details: > - Modified: _GremlinQueryComposer_: Continue composing queries using existing > claues. Additions alone. > - New: _DSLQueryExecutor_ Instantiates old or new executor based on flag. > - Modifed: _EntityDiscoveryService_ uses the new exectors. > - New _GremlinToTraversalTranslator_: Converts Gremlin clauses to > _AtlasGraphTraversal_. > - Modified: _AtlasGraphTraversal_: Additional methods. > - New: _SelectClauseProjections_: All queries with select operations. > > **Configuration** > _atlas.dsl.executor.traversal_ > - true: Uses the traversal-based implementation for query execution. > - false: Uses the script-engine based implementation for query execution. > > **Credits** > - Apoorv Naik (apoorvnaik): Original implementation. > - Nikhil Bonte (nbonte): Continued on Apoorv's implementation and addressed > short-comings. > > > Diffs > ----- > > > graphdb/api/src/main/java/org/apache/atlas/repository/graphdb/AtlasGraph.java > c016f6340 > > graphdb/api/src/main/java/org/apache/atlas/repository/graphdb/AtlasGraphTraversal.java > 881bb1e0f > > graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraph.java > 0dd573b89 > > graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphTraversal.java > c33c4f4d0 > intg/src/main/java/org/apache/atlas/AtlasConfiguration.java ea9f26d47 > > repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java > 01a6c303a > repository/src/main/java/org/apache/atlas/query/AtlasDSL.java b8a744b35 > repository/src/main/java/org/apache/atlas/query/DSLVisitor.java 700da955f > repository/src/main/java/org/apache/atlas/query/GremlinClause.java > 9704b0f9e > repository/src/main/java/org/apache/atlas/query/GremlinClauseList.java > 9f30e4dc0 > repository/src/main/java/org/apache/atlas/query/GremlinQuery.java 531f7ae86 > repository/src/main/java/org/apache/atlas/query/GremlinQueryComposer.java > 36b514e84 > repository/src/main/java/org/apache/atlas/query/SelectClauseComposer.java > 969fcd2b1 > > repository/src/main/java/org/apache/atlas/query/executors/DSLQueryExecutor.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/query/executors/GremlinClauseToTraversalTranslator.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/query/executors/ScriptEngineBasedExecutor.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/query/executors/SelectClauseProjections.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/query/executors/TraversalBasedExecutor.java > PRE-CREATION > repository/src/test/java/org/apache/atlas/query/DSLQueriesTest.java > 3bb3b07bf > > > Diff: https://reviews.apache.org/r/73128/diff/8/ > > > Testing > ------- > > **Unit Tests** > - Additional tests. > - _GremlinCompoerTest_ continues to validate the clause creation. > - Improved validation for _minMaxCount_ tests. > - Refactored: _TableValidator_ Validates output from a query with select > claues. > > **Volume Tests** > - Old implementation: 528 queries take over 10 mins to execute. > - New implemeentations: 528 queries execute in 7 secs. > > > Thanks, > > Ashutosh Mestry > >
