----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64721/#review194178 -----------------------------------------------------------
Fix it, then Ship it! Error handling need to be reviewed and updated. For example, there is no error for invalid queries like - hive_table invalid expression - hive_table invalid=expression - hive_table isa NonExistingTag I would suggest to handled these in a subsequent patch, as this patch adds sizable functionality to DSL. repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java Line 933 (original), 937 (patched) <https://reviews.apache.org/r/64721/#comment272898> "isNoneEmpty()" doesn't seem correct here. Consider replacing with "isNotEmpty()". repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java Line 937 (original), 948 (patched) <https://reviews.apache.org/r/64721/#comment272899> "isNoneEmpty()" doesn't seem correct here. Consider replacing with "isNotEmpty()". repository/src/main/java/org/apache/atlas/query/IdentifierHelper.java Lines 32 (patched) <https://reviews.apache.org/r/64721/#comment272900> if this method strips leading & trailing quotes, consider renaming this method, like: stripQuotes(identifier). repository/src/main/java/org/apache/atlas/query/IdentifierHelper.java Lines 73 (patched) <https://reviews.apache.org/r/64721/#comment272902> Wouldn't this replace() all quotes in the string. If the indent is to strip leading and trailing quotes, consider doing that explicitly. repository/src/main/java/org/apache/atlas/query/QueryProcessor.java Lines 58 (patched) <https://reviews.apache.org/r/64721/#comment272910> Given 'context' already holds reference to 'lookup', it will help to avoid having another reference here. Also, both 'context' and 'lookup' objects have references to 'errorList' object. It will be cleaner to have one of these objects own the object. repository/src/main/java/org/apache/atlas/query/antlr4/AtlasDSLLexer.java Line 1 (original), 1 (patched) <https://reviews.apache.org/r/64721/#comment272904> Please remove references to username - Madhan Neethiraj On Dec. 19, 2017, 8:10 p.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64721/ > ----------------------------------------------------------- > > (Updated Dec. 19, 2017, 8:10 p.m.) > > > Review request for atlas, Apoorv Naik, Madhan Neethiraj, and Sarath > Subramanian. > > > Bugs: ATLAS-2229 > https://issues.apache.org/jira/browse/ATLAS-2229 > > > Repository: atlas > > > Description > ------- > > **Background** > This implementation builds on top of the earlier Antlr-based DSL > implementation. > > **Scope** > Includes: > - New framework. > - Operators. > - Enhancements to _select_ clause. > - V1 REST APIs now integrated to call this implementation. > - Unit test suite. > > _Supported Operators_ > These are the currently supported operators: <, >, <=, >=, =, !=, in, like > > _New Framework_ > The key classes: > _QueryProcessor_ forms an entry point to query translation process. > _Context_ Maintains the currently active type. > _Lookup_ Provides a facade over _AtlasTypeRegistry_. It provides methods > useful during the query creation. > _Advice_ Uses _Context_ and services of _Lookup_ to provide consumer with > useful primitives to help make decisions during query creation. > > _Enhancements to Select Clause_ > This update adds support for 'as' clause within _select_. E.g. hive_table > where name = "Sales" select name as n, owner as o. > > _V1 REST APIs_ > The methods that support the V1 REST APIs have been updated to use the new > implementation. > > _Unit test suite_ > Unit tests written for DSL in branch-0.8 have now been moved to master. These > tests offer extensive coverage. These are over 200 in number. > > > Diffs > ----- > > intg/src/main/java/org/apache/atlas/model/discovery/SearchParameters.java > b816e805 > > repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java > cbc08b8c > repository/src/main/java/org/apache/atlas/query/DSLVisitor.java a2c6b983 > repository/src/main/java/org/apache/atlas/query/IdentifierHelper.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/query/Lookup.java PRE-CREATION > repository/src/main/java/org/apache/atlas/query/QueryProcessor.java > 60480a10 > repository/src/main/java/org/apache/atlas/query/antlr4/AtlasDSLLexer.java > 85f8d61f > repository/src/main/java/org/apache/atlas/query/antlr4/AtlasDSLParser.g4 > bf6b7e30 > repository/src/main/java/org/apache/atlas/query/antlr4/AtlasDSLParser.java > 73627a3f > > repository/src/main/java/org/apache/atlas/query/antlr4/AtlasDSLParserBaseVisitor.java > 4495f779 > > repository/src/main/java/org/apache/atlas/query/antlr4/AtlasDSLParserVisitor.java > 4985f8a1 > repository/src/test/java/org/apache/atlas/query/BasicTestSetup.java > 2aecf2b4 > repository/src/test/java/org/apache/atlas/query/DSLQueriesTest.java > a794a2ae > repository/src/test/java/org/apache/atlas/query/QueryProcessorTest.java > d1a3d108 > > webapp/src/main/java/org/apache/atlas/web/resources/MetadataDiscoveryResource.java > 0ac4ad56 > > > Diff: https://reviews.apache.org/r/64721/diff/1/ > > > Testing > ------- > > **Unit tests** > - Added 213 suite of tests that exercise the implementation. > - Updated _QueryProcessTest_ to include additional scenarios. > > **Functional tests** > - Verified scenarios from UI. > > > Thanks, > > Ashutosh Mestry > >
