----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43649/#review120281 -----------------------------------------------------------
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (lines 294 - 300) <https://reviews.apache.org/r/43649/#comment181941> Seems this is just a wrapper class with no extra functionality. I think it can be removed lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (lines 362 - 383) <https://reviews.apache.org/r/43649/#comment181942> Seems duplication from ES Driver. Can you try and refactor so that duplication is minimized? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ColumnSchema.java (line 27) <https://reviews.apache.org/r/43649/#comment181943> Driver-agnostic. Can go in lens-server-api. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (lines 189 - 191) <https://reviews.apache.org/r/43649/#comment181944> Duplicated from es driver, please refactor. Suggestion: put the function in es driver lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (lines 208 - 210) <https://reviews.apache.org/r/43649/#comment181714> Are we ever setting driver start and driver end times? lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidClient.java (line 49) <https://reviews.apache.org/r/43649/#comment181945> Please use one of `Mode` or `Type` everywhere. Right now it's confusing: `QueryMode getQueryType` `GroupByQueryType extends QueryMode` `TopNQueryType extends QueryMode` lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidClient.java (lines 62 - 72) <https://reviews.apache.org/r/43649/#comment181947> QueryMode as an abstract class is not useful. It can just as well be an interface. See http://stackoverflow.com/a/4794345 lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidClient.java (lines 74 - 96) <https://reviews.apache.org/r/43649/#comment181949> Didn't understand the need of these classes. They are both same and wrappers over one abstract method. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidResultSet.java (line 30) <https://reviews.apache.org/r/43649/#comment181950> Again, a copy of ESResultSet. Please refactor. lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidCriteriaVisitorFactory.java (line 22) <https://reviews.apache.org/r/43649/#comment181951> Please avoid the package name `lib`. Either everything is a library or nothing is. lens-driver-druid/src/test/java/org/apache/lens/driver/druid/QueryTranslationTest.java (line 95) <https://reviews.apache.org/r/43649/#comment181952> It's test code. an IOException will also indicate failure. There's no need to wrap it in a RuntimeException. lens-driver-druid/src/test/java/org/apache/lens/driver/druid/ResultSetTransformationTest.java (line 44) <https://reviews.apache.org/r/43649/#comment181953> We use testng project wide. Please use testng and remove junit dependencies. lens-driver-druid/src/test/resources/valid-queries.data (line 33) <https://reviews.apache.org/r/43649/#comment181954> redundant space? lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/impl/ESAggregateVisitor.java (lines 97 - 101) <https://reviews.apache.org/r/43649/#comment181955> ESVisitor is an abstract class. That can contain this function. Doing that will remove the changes done in this class and ESTermVisitor. lens-server-api/src/main/java/org/apache/lens/server/api/driver/lib/ASTVisitor.java (line 19) <https://reviews.apache.org/r/43649/#comment181956> Change package name. `lib` is not descriptive enough for code-readers. - Rajat Khandelwal On Feb. 18, 2016, 5:44 p.m., Rajitha R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43649/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2016, 5:44 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-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/ColumnSchema.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/DruidResultSet.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/Aggregators.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/LogicalOperators.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/Predicates.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/having/HavingLogicalOperators.java > PRE-CREATION > > lens-driver-druid/src/main/java/com/apache/lens/driver/druid/grammar/having/HavingPredicates.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/ESDriverConfig.java > 8f293f5 > > 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/main/java/org/apache/lens/driver/es/translator/impl/ESTermVisitor.java > 49aa0d4 > > lens-driver-es/src/test/java/org/apache/lens/driver/es/ResultSetTransformationTest.java > 0b78639 > 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/lib/ASTCriteriaVisitor.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/lib/ASTVisitor.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/lib/CriteriaVisitorFactory.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/driver/lib/exception/InvalidQueryException.java > PRE-CREATION > pom.xml b3afa59 > src/site/apt/admin/druiddriver-config.apt PRE-CREATION > > Diff: https://reviews.apache.org/r/43649/diff/ > > > Testing > ------- > > [INFO] Reactor Summary: > [INFO] > [INFO] Lens Checkstyle Rules ............................. SUCCESS [1.883s] > [INFO] Lens .............................................. SUCCESS [3.355s] > [INFO] Lens API .......................................... SUCCESS [25.446s] > [INFO] Lens API for server and extensions ................ SUCCESS [19.944s] > [INFO] Lens Cube ......................................... SUCCESS > [10:55.222s] > [INFO] Lens DB storage ................................... SUCCESS [20.357s] > [INFO] Lens Query Library ................................ SUCCESS [15.574s] > [INFO] Lens Hive Driver .................................. SUCCESS [2:49.465s] > [INFO] Lens Driver for JDBC .............................. SUCCESS [37.598s] > [INFO] Lens Elastic Search Driver ........................ SUCCESS [16.879s] > [INFO] Lens Driver for Druid ............................. SUCCESS [27.236s] > [INFO] Lens Server ....................................... SUCCESS > [10:03.541s] > [INFO] Lens client ....................................... SUCCESS [35.725s] > [INFO] Lens CLI .......................................... SUCCESS [50.380s] > [INFO] Lens Examples ..................................... SUCCESS [8.416s] > [INFO] Lens Ship Jars to Distributed Cache ............... SUCCESS [0.701s] > [INFO] Lens Distribution ................................. SUCCESS [8.340s] > [INFO] Lens ML Lib ....................................... SUCCESS [1:20.841s] > [INFO] Lens ML Ext Distribution .......................... SUCCESS [1.998s] > [INFO] Lens Regression ................................... SUCCESS [14.291s] > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 29:58.174s > [INFO] Finished at: Wed Feb 17 11:13:45 UTC 2016 > [INFO] Final Memory: 199M/1276M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Rajitha R > >
