You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lens.apache.org by Rajat Khandelwal <ra...@gmail.com> on 2016/05/02 14:01:58 UTC
Re: Review Request 43649: Druid driver in Lens
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43649/#review131251
-----------------------------------------------------------
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 83)
<https://reviews.apache.org/r/43649/#comment195153>
Remove this?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQueryBuilder.java (line 45)
<https://reviews.apache.org/r/43649/#comment195155>
Return type can be `GroupByDruidQuery`
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQueryBuilder.java (line 67)
<https://reviews.apache.org/r/43649/#comment195156>
Return type can be `TopNDruidQuery`
lens-api/src/main/resources/lens-errors.conf (line 349)
<https://reviews.apache.org/r/43649/#comment195165>
Message shouldn't be same as the message for another error. Otherwise it's pointless to add a new error code.
lens-cube/src/main/java/org/apache/lens/cube/parse/HQLParser.java (lines 846 - 852)
<https://reviews.apache.org/r/43649/#comment195166>
I'm not comfortable with having such a function in `HQLParser`. Getting first child of any Node shouldn't be throwing `InvalidQueryException`. The function name seems generic enough, and yet the use case will be very limited, since the assumption (that being unable to get first child means query is invalid) is wrong.
lens-cube/src/main/java/org/apache/lens/cube/parse/HQLParser.java (lines 865 - 867)
<https://reviews.apache.org/r/43649/#comment195167>
Should be removed.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 111)
<https://reviews.apache.org/r/43649/#comment195168>
This is `getName` in `ASTNode`:
```
@Override
public String getName() {
return String.valueOf(super.getToken().getType());
}
```
where `super.getToken().getType()` returns `int`.
seems `getName` won't be much informative in the error message.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 114)
<https://reviews.apache.org/r/43649/#comment195170>
We shouldn't use catch-all blocks. Instead, catch only the exceptions that are thrown in your try block. catch-all will catch the `RuntimeException`s too, and that's not desirable.
Secondly, this will also catch `InvalidQueryException`s, which will make your code throw an `InvalidQueryException` masking another `InvalidQueryException`, whereas you could have let the original `InvalidQueryException` pass through instead of wrapping it.
http://stackoverflow.com/questions/2416316/why-is-the-catchexception-almost-always-a-bad-idea
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 136)
<https://reviews.apache.org/r/43649/#comment195169>
Can the error message be a bit more descriptive?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 151)
<https://reviews.apache.org/r/43649/#comment195171>
Same catch-all issue. The code doesn't seem to be throwing any exceptions other than `InvalidQueryException`, and wrapping that is a bad idea.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (lines 167 - 169)
<https://reviews.apache.org/r/43649/#comment195172>
Can be removed.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (lines 222 - 230)
<https://reviews.apache.org/r/43649/#comment195173>
Can the switch-case and type-cast be moved to polymorphism?
1. That'll remove the unnecessary `default` case.
2. New sub-types of Criteria Type will be able to add code in their own class, instead of having to additionally add here too.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 247)
<https://reviews.apache.org/r/43649/#comment195174>
catch-all
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/ASTTraverserForDruid.java (line 271)
<https://reviews.apache.org/r/43649/#comment195175>
catch-all. Besides, the error message `"Exception while parsing order by"` will leave the person who's debugging looking for the cause. Why not let the cause through instead of wrapping it.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 81)
<https://reviews.apache.org/r/43649/#comment195198>
This cache will keep on growing. We should instead use Guava's cache classes.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 83)
<https://reviews.apache.org/r/43649/#comment195176>
Can be removed?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 94)
<https://reviews.apache.org/r/43649/#comment195177>
The last config can't be the default config. We need to add `druiddriver-site.xml` too.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 105)
<https://reviews.apache.org/r/43649/#comment195178>
Shouldn't be needed. We can require the constructor to be public instead of forcing it.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 191)
<https://reviews.apache.org/r/43649/#comment195181>
Ironic name, where the word `Complete` is not complete.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (lines 194 - 196)
<https://reviews.apache.org/r/43649/#comment195182>
Instead of catch, we should first check for existence in the map.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (line 217)
<https://reviews.apache.org/r/43649/#comment195223>
`context.getHiveConf` shouldn't be used to get instance of `CubeMetastoreClient`. That'd be constructing a client for each query context.
Although, this is the approach being taken by jdbc driver too, so I'm hoping someone else also does a review and we have a clarification.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (lines 296 - 298)
<https://reviews.apache.org/r/43649/#comment195199>
Check existence in the map instead of try-catch.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriver.java (lines 319 - 321)
<https://reviews.apache.org/r/43649/#comment195200>
Check existence instead of catch NPE
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidDriverConfig.java (lines 34 - 36)
<https://reviews.apache.org/r/43649/#comment195201>
As discussed, you're caching one configuration in your config class, either cache all, or cache none.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQuery.java (line 40)
<https://reviews.apache.org/r/43649/#comment195203>
Needed anymore? Or polymorphism is enough?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQuery.java (line 43)
<https://reviews.apache.org/r/43649/#comment195204>
Can we add Generic type?
I see the pattern in druid classes is the following:
`public class GroupByQuery extends BaseQuery<Row>`
`public class TopNQuery extends BaseQuery<Result<TopNResultValue>>`
In our classes too, `List<Row>` and `List<Result<TopNResultValue>>` are being used.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQuery.java (line 45)
<https://reviews.apache.org/r/43649/#comment195202>
Shouldn't this function be local to one subclass?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQuery.java (line 62)
<https://reviews.apache.org/r/43649/#comment195209>
Can the argument be more specific than `Object`?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/DruidQueryBuilder.java (line 45)
<https://reviews.apache.org/r/43649/#comment195208>
The functions in this class are hiding an explicit api which takes fluent arguments and hides them in an implicit api only taking positional arguments. Can something be done about that?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/GroupByDruidQuery.java (line 37)
<https://reviews.apache.org/r/43649/#comment195205>
Argument type can be subclass.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/GroupByDruidQuery.java (line 59)
<https://reviews.apache.org/r/43649/#comment195210>
Return type here can be made `GroupByResultTransformer`
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/TopNDruidQuery.java (line 46)
<https://reviews.apache.org/r/43649/#comment195211>
Return type can be made `TopNResultTransformer`
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/DruidClient.java (lines 35 - 37)
<https://reviews.apache.org/r/43649/#comment195213>
Can you add comments as to what is the purpose of each config.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/GroupByResultTransformer.java (line 49)
<https://reviews.apache.org/r/43649/#comment195214>
Can we avoid generic `Object` types everywhere?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/GroupByResultTransformer.java (line 51)
<https://reviews.apache.org/r/43649/#comment195215>
Avoid type cast.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/TopNResultTransformer.java (line 52)
<https://reviews.apache.org/r/43649/#comment195216>
Proper types to avoid type-casting.
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidCriteriaVisitor.java (line 61)
<https://reviews.apache.org/r/43649/#comment195218>
Can we avoid type-cast?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidHavingVisitor.java (line 61)
<https://reviews.apache.org/r/43649/#comment195219>
Avoid type-cast
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidVisitor.java (line 165)
<https://reviews.apache.org/r/43649/#comment195224>
Can we avoid typecast?
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/DruidVisitor.java (line 187)
<https://reviews.apache.org/r/43649/#comment195225>
Prone to NPE when table doesn't have property `DruidDriverConfig.DRUID_TABLE_TIME_DIMENSION`
lens-driver-druid/src/main/java/com/apache/lens/driver/druid/translator/GroupByVisitor.java (line 105)
<https://reviews.apache.org/r/43649/#comment195226>
Can we avoid typecast?
lens-driver-druid/src/test/java/org/apache/lens/driver/druid/QueryTranslationTest.java (lines 167 - 168)
<https://reviews.apache.org/r/43649/#comment195227>
Remove logs, as the queries will be visible in failure cases.
lens-driver-druid/src/test/java/org/apache/lens/driver/druid/QueryTranslationTest.java (line 169)
<https://reviews.apache.org/r/43649/#comment195228>
Test case naming can be done dynamically. http://biggerwrench.blogspot.in/2014/02/testng-dynamically-naming-tests-from.html
lens-driver-druid/src/test/java/org/apache/lens/driver/druid/QueryTranslationTest.java (line 208)
<https://reviews.apache.org/r/43649/#comment195229>
`private`
lens-driver-druid/src/test/resources/hive-site.xml (lines 26 - 36)
<https://reviews.apache.org/r/43649/#comment195230>
Formatting
lens-driver-druid/src/test/resources/valid-queries.data (line 50)
<https://reviews.apache.org/r/43649/#comment195231>
The value of `expectedJson` doesn't seem to be json.
lens-server-api/src/main/java/org/apache/lens/server/api/error/LensDriverErrorCode.java (line 37)
<https://reviews.apache.org/r/43649/#comment195232>
Getting used anywhere? If yes, `getErrorCode` would be more descriptive name.
- Rajat Khandelwal
On April 29, 2016, 8:34 p.m., Rajitha R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43649/
> -----------------------------------------------------------
>
> (Updated April 29, 2016, 8:34 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-api/src/main/resources/lens-errors.conf 395d63b
> 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/GroupByDruidQuery.java PRE-CREATION
> lens-driver-druid/src/main/java/com/apache/lens/driver/druid/TopNDruidQuery.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/client/GroupByResultTransformer.java PRE-CREATION
> lens-driver-druid/src/main/java/com/apache/lens/driver/druid/client/TopNResultTransformer.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/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-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java eef4464
> 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/AbstractLensDriver.java 883ad9d
> 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/DefaultInMemoryResultSet.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/driver/ast/exception/UnSupportedQueryException.java PRE-CREATION
> lens-server-api/src/main/java/org/apache/lens/server/api/error/LensDriverErrorCode.java 0c6257b
> 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
> src/site/apt/user/client-config.apt 714db18
>
> Diff: https://reviews.apache.org/r/43649/diff/
>
>
> Testing
> -------
>
> [INFO] Lens Checkstyle Rules ............................. SUCCESS [2.151s]
> [INFO] Lens .............................................. SUCCESS [6.365s]
> [INFO] Lens API .......................................... SUCCESS [23.564s]
> [INFO] Lens API for server and extensions ................ SUCCESS [17.259s]
> [INFO] Lens Cube ......................................... SUCCESS [12:03.449s]
> [INFO] Lens DB storage ................................... SUCCESS [20.300s]
> [INFO] Lens Query Library ................................ SUCCESS [12.538s]
> [INFO] Lens Hive Driver .................................. SUCCESS [2:53.916s]
> [INFO] Lens Driver for JDBC .............................. SUCCESS [43.823s]
> [INFO] Lens Elastic Search Driver ........................ SUCCESS [16.896s]
> [INFO] Lens Driver for Druid ............................. SUCCESS [26.296s]
> [INFO] Lens Server ....................................... SUCCESS [17:59.683s]
> [INFO] Lens client ....................................... SUCCESS [35.523s]
> [INFO] Lens CLI .......................................... SUCCESS [2:33.505s]
> [INFO] Lens Examples ..................................... SUCCESS [7.645s]
> [INFO] Lens Ship Jars to Distributed Cache ............... SUCCESS [0.685s]
> [INFO] Lens Distribution ................................. SUCCESS [9.048s]
> [INFO] Lens Regression ................................... SUCCESS [10.181s]
> [INFO] Lens UI ........................................... SUCCESS [3.198s]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 39:26.518s
> [INFO] Finished at: Fri Apr 29 19:40:08 IST 2016
> [INFO] Final Memory: 144M/1496M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Rajitha R
>
>