----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43649/#review126812 -----------------------------------------------------------
lens-cube/src/main/java/org/apache/lens/cube/parse/HQLParser.java (lines 865 - 867) <https://reviews.apache.org/r/43649/#comment190549> Let's not wrap e in `InvalidQueyException`. e is anyway `LensException`, just propagate that. What do you think? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (lines 113 - 114) <https://reviews.apache.org/r/43649/#comment190553> What exceptions can occur at this point? Is it possible to get a more descriptive error message, as this error will ultimately travel till the querying user. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 175) <https://reviews.apache.org/r/43649/#comment190555> Is it possible to wrap `HiveException` in an `InvalidQueryException`? check where HiveException occurs and whether that indicates invalid query. Applicable for all functions below where `HiveException` is in the `throws` list lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (lines 184 - 188) <https://reviews.apache.org/r/43649/#comment190554> Seeing these type-castings, should the field visitor be declared as an instance of DruidVisitor instead of ASTVisitor? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 191) <https://reviews.apache.org/r/43649/#comment190556> Can we throw `InvalidQueryException` here too instead of `RuntimeException`? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 229) <https://reviews.apache.org/r/43649/#comment190557> remove `this`. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 230) <https://reviews.apache.org/r/43649/#comment190558> redundant `toString` call. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (lines 330 - 331) <https://reviews.apache.org/r/43649/#comment190561> `and` and `AND` are supported, what about other combinations, like `aNd`, `AnD`, `aND` etc? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 134) <https://reviews.apache.org/r/43649/#comment190562> Let's throw `LensException` here. You'll need to create something like `LensDriverErrorCode.NOT_SUPPORTED`, add a new error message in `lens-errors.conf`. Then you can `throw new LensException(NOT_SUPPORTED.getErrorInfo(), "operation_name")` lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (lines 144 - 149) <https://reviews.apache.org/r/43649/#comment190563> Throw `LensException` like mentioned earlier. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 207) <https://reviews.apache.org/r/43649/#comment190564> `updateStatus` is called by status updater thread, this line will update status on every call. start time will keep varying till the query finishes. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 218) <https://reviews.apache.org/r/43649/#comment190565> Should only set if not already set. The first time `isDone()` is true, is when finish time should be set. Even better if future object allows asynchronous callback as soon as it's finished. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (lines 260 - 270) <https://reviews.apache.org/r/43649/#comment190567> Let's check for null case early instead of relying on `NPE`. http://stackoverflow.com/a/18266490/459384 lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (lines 284 - 287) <https://reviews.apache.org/r/43649/#comment190568> This function is empty in 2 of the 3 existing drivers, and is empty in the new driver too. Maybe we can move it up to `AbstractLensDriver` lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriverConfig.java (line 37) <https://reviews.apache.org/r/43649/#comment190957> do we need to cache the dateformat in a field, or can we fetch from conf always? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQueryBuilder.java (lines 64 - 84) <https://reviews.apache.org/r/43649/#comment190958> can we have DruidQuery as abstract class and have sub classes for groupby and topn. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidClientImpl.java (lines 79 - 85) <https://reviews.apache.org/r/43649/#comment190959> This can go in the class heirarchy mentioned in the other comment. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidResultSetTransformer.java (lines 157 - 161) <https://reviews.apache.org/r/43649/#comment190961> Such type checks can be moved to polymorphism. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/exceptions/DruidClientException.java (line 26) <https://reviews.apache.org/r/43649/#comment190962> can we have default error info as `LensDriverErrorCode.DRIVER_ERROR` lens-driver-druid/src/main/java/com/apache/lens/driver/druid/exceptions/DruidRewriteException.java (line 27) <https://reviews.apache.org/r/43649/#comment190963> `LensDriverErrorCode.SEMANTIC_ERROR` as defalt error info. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/LogicalOperator.java (line 72) <https://reviews.apache.org/r/43649/#comment190964> `and` and `AND` are handled, so are `or` and `OR`. but `NOT` is not handled. Also, what about jumbled cases like `aNd`. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/having/HavingLogicalOperator.java (line 32) <https://reviews.apache.org/r/43649/#comment190965> can we name them `AND`, `NOT` and `OR` in capital case? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/having/HavingPredicate.java (line 49) <https://reviews.apache.org/r/43649/#comment190966> I'm wondering, don't we also need `GREATER_THAN_OR_EQUAL`, and `LESS_THAN_OR_EQUAL`? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidHavingVisitor.java (line 61) <https://reviews.apache.org/r/43649/#comment190969> Can we avoid type casting somehow? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidVisitor.java (lines 78 - 80) <https://reviews.apache.org/r/43649/#comment190970> can't we import `DateTime`? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidVisitor.java (line 84) <https://reviews.apache.org/r/43649/#comment190971> config constant can be kept in `DruidDriverConfig` lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidVisitor.java (line 153) <https://reviews.apache.org/r/43649/#comment190972> InvalidQueryException lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidVisitor.java (line 174) <https://reviews.apache.org/r/43649/#comment190973> `InvalidQueryException` lens-driver-druid/src/test/java/org/apache/lens/driver/druid/QueryTranslationTest.java (line 110) <https://reviews.apache.org/r/43649/#comment190975> `INVALID_QUERIES` lens-driver-druid/src/test/java/org/apache/lens/driver/druid/QueryTranslationTest.java (line 132) <https://reviews.apache.org/r/43649/#comment190974> shouldn't we stop the test if tables are not created? lens-driver-druid/src/test/java/org/apache/lens/driver/druid/QueryTranslationTest.java (line 168) <https://reviews.apache.org/r/43649/#comment190976> can we use a dataProvider? so that all tests can run independently. Right now they are running sequentially introducing dependency of later tests on first few tests. lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/ESVisitor.java (line 182) <https://reviews.apache.org/r/43649/#comment190967> `InvalidQueryException`? lens-server-api/src/main/java/org/apache/lens/server/api/driver/DefaultResultSet.java (line 27) <https://reviews.apache.org/r/43649/#comment190968> `DefaultInMemoryResultSet` lens-server-api/src/main/java/org/apache/lens/server/api/driver/ast/exception/InvalidQueryException.java (line 27) <https://reviews.apache.org/r/43649/#comment190552> can we pass `SEMANTIC_ERROR.getLensErrorInfo()` as the default error info -- if not provided -- in `super(...)` calls in all constructors? - Rajat Khandelwal On March 29, 2016, 6:30 p.m., Rajitha R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43649/ > ----------------------------------------------------------- > > (Updated March 29, 2016, 6:30 p.m.) > > > Review request for lens and Amareshwari Sriramadasu. > > > Bugs: LENS-271 > https://issues.apache.org/jira/browse/LENS-271 > > > Repository: lens > > > Description > ------- > > Changes for adding Druid driver in Lens > > > Diffs > ----- > > lens-cube/src/main/java/org/apache/lens/cube/parse/HQLParser.java 8d6105f > lens-driver-druid/pom.xml PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriverConfig.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQuery.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQueryBuilder.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidClient.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidClientImpl.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidResultSetTransformer.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/exceptions/DruidClientException.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/exceptions/DruidRewriteException.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/Aggregator.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/LogicalOperator.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/Predicate.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/having/HavingLogicalOperator.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/having/HavingPredicate.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidCriteriaVisitor.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidCriteriaVisitorFactory.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidHavingVisitor.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidHavingVisitorFactory.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidVisitor.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/GroupByVisitor.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/TopNVisitor.java > PRE-CREATION > lens-driver-druid/src/main/resources/druiddriver-default.xml PRE-CREATION > > lens-driver-druid/src/test/java/org/apache/lens/driver/druid/DruidInitDriverTest.java > PRE-CREATION > > lens-driver-druid/src/test/java/org/apache/lens/driver/druid/MockClientDruid.java > PRE-CREATION > > lens-driver-druid/src/test/java/org/apache/lens/driver/druid/QueryTranslationTest.java > PRE-CREATION > > lens-driver-druid/src/test/java/org/apache/lens/driver/druid/ResultSetTransformationTest.java > PRE-CREATION > lens-driver-druid/src/test/resources/druiddriver-default.xml PRE-CREATION > lens-driver-druid/src/test/resources/hive-site.xml PRE-CREATION > lens-driver-druid/src/test/resources/invalid-queries.data PRE-CREATION > lens-driver-druid/src/test/resources/valid-queries.data PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/ASTTraverserForES.java > 07b157e > lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java > 8a4f410 > lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriverConfig.java > 8f293f5 > lens-driver-es/src/main/java/org/apache/lens/driver/es/client/ESClient.java > 5363a94 > > lens-driver-es/src/main/java/org/apache/lens/driver/es/client/ESResultSet.java > b59949b > > lens-driver-es/src/main/java/org/apache/lens/driver/es/client/jest/JestClientImpl.java > 35dc070 > > lens-driver-es/src/main/java/org/apache/lens/driver/es/client/jest/JestResultSetTransformer.java > 38d91f9 > > lens-driver-es/src/main/java/org/apache/lens/driver/es/exceptions/InvalidQueryException.java > 20634af > > lens-driver-es/src/main/java/org/apache/lens/driver/es/grammar/Aggregations.java > f726fa5 > > lens-driver-es/src/main/java/org/apache/lens/driver/es/grammar/LogicalOperators.java > b9cf000 > > lens-driver-es/src/main/java/org/apache/lens/driver/es/grammar/Predicates.java > ec2af0f > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/ASTCriteriaVisitor.java > b429424 > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/ASTVisitor.java > 77e774f > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/CriteriaVisitorFactory.java > 92ec10f > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/ESVisitor.java > 441f6d6 > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/impl/ESAggregateVisitor.java > e8f2cea > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/impl/ESCriteriaVisitor.java > d1bf2a4 > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/impl/ESCriteriaVisitorFactory.java > 04b773d > lens-driver-es/src/test/java/org/apache/lens/driver/es/MockClientES.java > 77300f9 > > lens-driver-es/src/test/java/org/apache/lens/driver/es/ResultSetTransformationTest.java > 0b78639 > > lens-driver-es/src/test/java/org/apache/lens/driver/es/ScrollingQueryTest.java > ea84d8c > lens-examples/src/main/resources/cube-queries.sql 9f4a353 > lens-examples/src/main/resources/dimension-queries.sql a5f51d9 > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/ColumnSchema.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/DefaultResultSet.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/ast/ASTCriteriaVisitor.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/ast/ASTVisitor.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/ast/CriteriaVisitorFactory.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/ast/exception/InvalidQueryException.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/AbstractQueryContext.java > b568ffb > pom.xml 309921f > src/site/apt/admin/druiddriver-config.apt PRE-CREATION > > Diff: https://reviews.apache.org/r/43649/diff/ > > > Testing > ------- > > [INFO] Lens Checkstyle Rules ............................. SUCCESS [1.858s] > [INFO] Lens .............................................. SUCCESS [3.221s] > [INFO] Lens API .......................................... SUCCESS [29.686s] > [INFO] Lens API for server and extensions ................ SUCCESS [19.187s] > [INFO] Lens Cube ......................................... SUCCESS > [11:01.656s] > [INFO] Lens DB storage ................................... SUCCESS [19.576s] > [INFO] Lens Query Library ................................ SUCCESS [15.811s] > [INFO] Lens Hive Driver .................................. SUCCESS [2:51.644s] > [INFO] Lens Driver for JDBC .............................. SUCCESS [36.149s] > [INFO] Lens Elastic Search Driver ........................ SUCCESS [16.321s] > [INFO] Lens Driver for Druid ............................. SUCCESS [25.900s] > [INFO] Lens Server ....................................... SUCCESS > [16:32.061s] > [INFO] Lens client ....................................... SUCCESS [36.089s] > [INFO] Lens CLI .......................................... SUCCESS [2:50.810s] > [INFO] Lens Examples ..................................... SUCCESS [10.117s] > [INFO] Lens Ship Jars to Distributed Cache ............... SUCCESS [1.600s] > [INFO] Lens Distribution ................................. SUCCESS [9.495s] > [INFO] Lens ML Lib ....................................... SUCCESS [1:19.685s] > [INFO] Lens ML Ext Distribution .......................... SUCCESS [1.849s] > [INFO] Lens Regression ................................... SUCCESS [14.310s] > [INFO] Lens UI ........................................... SUCCESS [28.467s] > > > Thanks, > > Rajitha R > >
