----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36434/#review91435 -----------------------------------------------------------
lens-driver-es/pom.xml (line 37) <https://reviews.apache.org/r/36434/#comment144799> Can probably be changed to `Lens Elastic Search Driver`. Let's look at lens hive driver pom: ``` <project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> <modelVersion>4.0.0</modelVersion> <name>Lens Hive Driver</name> <parent> <artifactId>apache-lens</artifactId> <groupId>org.apache.lens</groupId> <version>2.3.0-beta-incubating-SNAPSHOT</version> </parent> <properties> <mvn.classpath.file>${pom.basedir}/target/classpath</mvn.classpath.file> </properties> <artifactId>lens-driver-hive</artifactId> <packaging>jar</packaging> <description>Hive execution driver</description> ``` lens-driver-es/pom.xml (line 51) <https://reviews.apache.org/r/36434/#comment144798> Do we need hive dependencies for elastic search? lens-driver-es/pom.xml (line 65) <https://reviews.apache.org/r/36434/#comment144797> Let's not use snapshot versions. lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java (line 19) <https://reviews.apache.org/r/36434/#comment144800> Let's change package name to `org.apache.lens.driver.es` or ``org.apache.lens.driver.elasticsearch` Reference: Other drivers' package names. An IDE should give all the suggestions on typing `org.apache.lens.driver.` lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java (line 62) <https://reviews.apache.org/r/36434/#comment144801> Can be replaced with `@RequiredArgsConstructor` lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java (line 81) <https://reviews.apache.org/r/36434/#comment144802> The chain is a little hard to understand. Can you either document or -- preferrably -- refactor out with descriptive name lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java (line 84) <https://reviews.apache.org/r/36434/#comment144803> Why this redundant next call? If it weren't there, we could have used foreach loop. lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java (line 107) <https://reviews.apache.org/r/36434/#comment144804> `default`? lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java (line 132) <https://reviews.apache.org/r/36434/#comment144805> Lot of code dependent on node values and sequence. I hope there are enough test cases to cover any bugs. lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java (lines 170 - 181) <https://reviews.apache.org/r/36434/#comment144806> Can we abstract out such code blocks instead of writing everything in `accept`? lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java (line 235) <https://reviews.apache.org/r/36434/#comment144807> Exception in new file please. And please see if it's possible to make it checked exception? I think in current design we wrap driver level exceptions inside LensException and then wrap that with http codes. lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java (line 57) <https://reviews.apache.org/r/36434/#comment144808> Let's create a new subclass of querycost. That way `Addable` won't be in `add` signature lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java (line 76) <https://reviews.apache.org/r/36434/#comment144810> We're returning 0 from both exec time and resource usage. Seems like we want to say that query cost on this driver is the lowest cost possible. Please consider if it's possible to use org.apache.lens.driver.jdbc.JDBCDriver.JDBC_DRIVER_COST instead of anonymous class. We'd like to have only one implementation of querycost across one deployment. lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java (lines 197 - 205) <https://reviews.apache.org/r/36434/#comment144812> What do empty implementation mean? Is it okay to do nothing? lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java (lines 260 - 267) <https://reviews.apache.org/r/36434/#comment144813> Let's at least implement these if we want persistence on restart. lens-driver-es/src/main/java/org/apache/lens/es/ESDriverConfig.java (line 32) <https://reviews.apache.org/r/36434/#comment144815> The naming is weird. It should be either `lensDriverEsMaxRowSize` or `lens.driver.es.max.row.size`. You've mixed two naming strategies. In the long term It's very very confusing for the end user. Let's follow what other drivers are following. lens-driver-es/src/main/java/org/apache/lens/es/ESDriverConfig.java (line 56) <https://reviews.apache.org/r/36434/#comment144816> Why do we need a constant for India specific time zone? lens-driver-es/src/main/java/org/apache/lens/es/client/ESResultSet.java (line 69) <https://reviews.apache.org/r/36434/#comment144818> Not a scope of this review request, but I think this should be changed to `throws UnsupportedOperationException` lens-driver-es/src/main/java/org/apache/lens/es/client/jest/JestClientImpl.java (line 85) <https://reviews.apache.org/r/36434/#comment144820> Let's create separate classes for Exceptions. And I don't think there's any need for creating unchecked exception. driver methods are allowed to throw LensException, so we can use inner checked exceptions and on interface methods, wrap them with LensException. lens-driver-es/src/main/java/org/apache/lens/es/client/jest/JestClientImpl.java (line 101) <https://reviews.apache.org/r/36434/#comment144821> I think we need to create a private default constructor here to avoid checkstyle errors. lens-driver-es/src/main/java/org/apache/lens/es/translator/ASTVisitor.java (line 28) <https://reviews.apache.org/r/36434/#comment144823> Does the enum need to be inside interface? Or can it be separate? lens-driver-es/src/test/java/org/apache/lens/es/ResultSetTransformationTest.java (line 487) <https://reviews.apache.org/r/36434/#comment144826> All these conf constants need not be always referred by string value. Let's define them at one place and use the Java String Object everywhere. lens-driver-es/src/test/java/org/apache/lens/es/ResultSetTransformationTest.java (lines 488 - 496) <https://reviews.apache.org/r/36434/#comment144824> Similar thing for these constants. lens-driver-es/src/test/java/org/apache/lens/es/ScrollingQueryTest.java (lines 44 - 53) <https://reviews.apache.org/r/36434/#comment144825> Constant strings as Java String Objects. lens-server/pom.xml (line 77) <https://reviews.apache.org/r/36434/#comment144827> This is the only change required in server? Don't we need to add es driver to the list of drivers in any conf? - Rajat Khandelwal On July 13, 2015, 9:37 a.m., Amruth Sampath wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36434/ > ----------------------------------------------------------- > > (Updated July 13, 2015, 9:37 a.m.) > > > Review request for lens, Amareshwari Sriramadasu, Jaideep dhok, and sharad > agarwal. > > > Bugs: LENS-252 > https://issues.apache.org/jira/browse/LENS-252 > > > Repository: lens > > > Description > ------- > > Elastic search driver for lens > ~~~~~~~~~~~~~~~~~~~~~~ > Elastic search accepts a nested json as a query and returns a json result. > The json result is nested for group by queries and simple for simple selects. > HQL -> ES json query > ~~~~~~~~~~~~~~~~~ > -> I have written a traversal (ASTTraverserForES)(specific for noSQL stores). > The traversal could be used for any purpose like query building/validation > etc. > -> The traversal will take in a query visitor (ASTVisitor.java) (for building > the query) and a criteria visitor (for building the where clause). I have > checked in concrete visitors for ElasticSearch that can build the elastic > search json query. > Elasticsearch client > ~~~~~~~~~~~~~~~ > -> There are multiple choices of elasticsearch client available. I've made > the client pluggable. > -> I've added one default HTTPClient implementation (Jest library - apache > 2). The choice of HTTP client over transport client was made because of the > version consistency requirement between the transport client and the ES > server. > -> Every client has to implement an execute method that takes in the query > and returns a LensResultSet. Hence the transformation of resultset must also > be done by the client implementation. > Elasticsearch Jest json response -> LensResultSet > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > The result set obtained could be a simple hit result (document response) or > facets response. > -> Facets response has a tree structure. Finding all the paths in the tree > will give us all the rows. Look at JestResultSetTransformer > (AggregateTransformer) > -> Hits response is straightforward to decode (TermTransformer) > Known issues/shortcommings > ~~~~~~~~~~~~~~~~~~~~~~~~ > -> Scrolling responses for aggregate queries (by design ES always returns the > complete bucket in a single json - there is no scroll facility) > -> Order by in aggregate queries. Fully functional order by queries can get > complex as the ordering by measure can happen only in the immediate parent > group by. 'Limit' is also blocked as it could be misleading to have limit > without order by. (Please note that order by and limit will still work in > queries without group bys) > -> *, count is not available as of now. > -> support for other UDFs. Right now common UDAFs like sum, min, max are > supported. We need a way to seamlessly translate a new UDF to elastic search > without code change > -> Query estimation > -> Session level config injection for properties like fetch size and group by > cardinality size? (Right now these configs are at driver level) > Have added a esdriver-default.xml for looking up default properties > > > Diffs > ----- > > lens-driver-es/pom.xml PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/es/ESDriverConfig.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/es/ESQuery.java PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/es/client/ESClient.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/es/client/ESResultSet.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/es/client/jest/JestClientImpl.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/es/client/jest/JestResultSetTransformer.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/es/translator/ASTCriteriaVisitor.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/es/translator/ASTVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/es/translator/CriteriaVisitorFactory.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/es/translator/ESVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/es/translator/impl/ESAggregateVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/es/translator/impl/ESCriteriaVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/es/translator/impl/ESCriteriaVisitorFactory.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/es/translator/impl/ESTermVisitor.java > PRE-CREATION > lens-driver-es/src/main/resources/esdriver-default.xml PRE-CREATION > lens-driver-es/src/test/java/org/apache/lens/es/ESDriverTest.java > PRE-CREATION > lens-driver-es/src/test/java/org/apache/lens/es/MockClientES.java > PRE-CREATION > lens-driver-es/src/test/java/org/apache/lens/es/QueryTranslationTest.java > PRE-CREATION > > lens-driver-es/src/test/java/org/apache/lens/es/ResultSetTransformationTest.java > PRE-CREATION > lens-driver-es/src/test/java/org/apache/lens/es/ScrollingQueryTest.java > PRE-CREATION > lens-server/pom.xml b85292c > pom.xml 4b049f0 > src/site/apt/user/cli.apt 353c171 > > Diff: https://reviews.apache.org/r/36434/diff/ > > > Testing > ------- > > Added unit test cases for testing > - query translation > - result set translation > - Scrolling > > > Thanks, > > Amruth Sampath > >
