----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36434/#review92234 -----------------------------------------------------------
lens-driver-es/src/main/java/org/apache/lens/driver/es/ASTTraverserForES.java (line 54) <https://reviews.apache.org/r/36434/#comment146285> Please add `@NonNull` wherever required. It'll ensure that the object is created in valid state. Also, if there are any other constraints regarding valid creation, let's use guava's (checkArgument)[http://docs.guava-libraries.googlecode.com/git-history/master/javadoc/com/google/common/base/Preconditions.html] with an explicit constructor replacing `RequiredArgsConstructor`. lens-driver-es/src/main/java/org/apache/lens/driver/es/ASTTraverserForES.java (line 211) <https://reviews.apache.org/r/36434/#comment146286> Could having `e` as cause of new Exception be helpful in debugging? lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java (line 165) <https://reviews.apache.org/r/36434/#comment146288> Would it be better to do an existence check in the map before calling `remove` instead of waiting on NPE? http://stackoverflow.com/questions/18265940/should-i-always-not-catch-nullpointerexception lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java (line 178) <https://reviews.apache.org/r/36434/#comment146287> Would it be more optimal to do an exists check before `remove` call instead of waiting for `NullPointerException`? lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java (line 191) <https://reviews.apache.org/r/36434/#comment146289> Same as above. lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java (line 227) <https://reviews.apache.org/r/36434/#comment146290> There are discussions on SO regarding catching NPEs: http://stackoverflow.com/questions/8965988/catching-null-pointer-exceptions http://stackoverflow.com/questions/15172367/ifnull-check-else-vs-try-catchnullpointerexception-which-is-more-efficient lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java (line 235) <https://reviews.apache.org/r/36434/#comment146291> http://stackoverflow.com/questions/2586290/is-catching-a-null-pointer-exception-a-code-smell lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java (line 282) <https://reviews.apache.org/r/36434/#comment146294> this should at best be a `debug` log. If you don't get any exceptions yet, it by default means that configure happened successfully. lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java (line 307) <https://reviews.apache.org/r/36434/#comment146295> Can we rename it to something more revealing? `QueryExecuteCallable` or something? lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriverConfig.java (lines 107 - 121) <https://reviews.apache.org/r/36434/#comment146296> Can be replaced by `@Getter` lens-driver-es/src/main/java/org/apache/lens/driver/es/ESQuery.java (line 26) <https://reviews.apache.org/r/36434/#comment146297> All fields are final with explicit `Getter`s. Consider using `@Data` with explicit constructor(since lists are being copied). lens-driver-es/src/main/java/org/apache/lens/driver/es/client/ESClient.java (lines 47 - 57) <https://reviews.apache.org/r/36434/#comment146298> Let's make it an interface instead of an abstract class which does nothing but set an instance level variable in constructor. The implementations should be free to use whatever is passed in constructor the way it wants. Some implementations might not need the parameter. The implementations should not be given the instance level field without them being aware of its availability. Put another way, `super(query)` and calling `this.esQuery = query` both take same number of lines and the latter is cleaner for the code readers. lens-driver-es/src/main/java/org/apache/lens/driver/es/client/ESClient.java (line 115) <https://reviews.apache.org/r/36434/#comment146299> What issue? At least pass `e` as cause. lens-driver-es/src/main/java/org/apache/lens/driver/es/client/ESClient.java (line 122) <https://reviews.apache.org/r/36434/#comment146300> Would `batch.next()` throw a more meaningful exception(other than a generic `RuntimeException`) in case `!hasNext()`? lens-driver-es/src/main/java/org/apache/lens/driver/es/client/jest/JestClientImpl.java (lines 45 - 49) <https://reviews.apache.org/r/36434/#comment146301> Can these be moved to `ESDriverConfig`? lens-driver-es/src/main/java/org/apache/lens/driver/es/client/jest/JestResultSetTransformer.java (lines 168 - 172) <https://reviews.apache.org/r/36434/#comment146302> If these are the only usages of the inner static Transformer classes, then I don't see the need to have these as subclasses of one common class, much less as subclssses of a class that wraps them. lens-driver-es/src/main/java/org/apache/lens/driver/es/exceptions/ESClientException.java (lines 23 - 27) <https://reviews.apache.org/r/36434/#comment146303> Can we put all constructors corresponding to other constructors of `LensException`? lens-driver-es/src/main/java/org/apache/lens/driver/es/exceptions/InvalidQueryException.java (line 26) <https://reviews.apache.org/r/36434/#comment146304> One more constructor with message and cause. lens-driver-es/src/main/resources/esdriver-default.xml (lines 62 - 70) <https://reviews.apache.org/r/36434/#comment146306> can we remove cube level properties from driver defaults? lens-driver-es/src/main/resources/esdriver-default.xml (line 71) <https://reviews.apache.org/r/36434/#comment146310> Can you look at `org.apache.lens.doc.TestGenerateConfigDoc` and make necessary changes there? lens-driver-es/src/test/java/org/apache/lens/driver/es/ResultSetTransformationTest.java (line 323) <https://reviews.apache.org/r/36434/#comment146307> Can we read this data(string) from a resource file? Would be more readable. lens-driver-es/src/test/java/org/apache/lens/driver/es/ResultSetTransformationTest.java (line 489) <https://reviews.apache.org/r/36434/#comment146309> Can probably have a `esdriver site xml` in resources and overlay `config` with that. - Rajat Khandelwal On July 19, 2015, 3:59 p.m., Amruth Sampath wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36434/ > ----------------------------------------------------------- > > (Updated July 19, 2015, 3:59 p.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/driver/es/ASTTraverserForES.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriver.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/driver/es/ESDriverConfig.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/driver/es/ESQuery.java > PRE-CREATION > lens-driver-es/src/main/java/org/apache/lens/driver/es/client/ESClient.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/client/ESResultSet.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/client/jest/JestClientImpl.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/client/jest/JestResultSetTransformer.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/exceptions/ESClientException.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/exceptions/InvalidQueryException.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/ASTCriteriaVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/ASTVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/CriteriaVisitorFactory.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/ESVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/impl/ESAggregateVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/impl/ESCriteriaVisitor.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/es/translator/impl/ESCriteriaVisitorFactory.java > PRE-CREATION > > lens-driver-es/src/main/java/org/apache/lens/driver/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/driver/es/ESDriverTest.java > PRE-CREATION > lens-driver-es/src/test/java/org/apache/lens/driver/es/MockClientES.java > PRE-CREATION > > lens-driver-es/src/test/java/org/apache/lens/driver/es/QueryTranslationTest.java > PRE-CREATION > > lens-driver-es/src/test/java/org/apache/lens/driver/es/ResultSetTransformationTest.java > PRE-CREATION > > lens-driver-es/src/test/java/org/apache/lens/driver/es/ScrollingQueryTest.java > PRE-CREATION > lens-server/pom.xml b85292c > pom.xml 4b049f0 > > Diff: https://reviews.apache.org/r/36434/diff/ > > > Testing > ------- > > Added unit test cases for testing > - query translation > - result set translation > - Scrolling > > > Thanks, > > Amruth Sampath > >
