> On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/pom.xml, line 51 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009170#file1009170line51> > > > > Do we need hive dependencies for elastic search?
Added dep on hive-service. Required by LensResultSetMetaData (ColumnDescriptor) > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/pom.xml, line 65 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009170#file1009170line65> > > > > Let's not use snapshot versions. Using the version of hive-service in main pom.xml > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java, > > line 81 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009171#file1009171line81> > > > > The chain is a little hard to understand. Can you either document or -- > > preferrably -- refactor out with descriptive name Have refactored ASTTraverserForES for more readability > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java, > > lines 170-181 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009171#file1009171line170> > > > > Can we abstract out such code blocks instead of writing everything in > > `accept`? Have refactored ASTTraverserForES for more readability. Check the new diff for more details > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java, > > line 132 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009171#file1009171line132> > > > > Lot of code dependent on node values and sequence. I hope there are > > enough test cases to cover any bugs. Have refactored ASTTraverserForES for more readability. Check the new diff for more details > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java, > > line 107 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009171#file1009171line107> > > > > `default`? Have refactored ASTTraverserForES for more readability. Check the new diff for more details > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java, > > line 84 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009171#file1009171line84> > > > > Why this redundant next call? If it weren't there, we could have used > > foreach loop. Have refactored ASTTraverserForES for more readability. Check the new diff for more details > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ASTTraverserForES.java, > > line 235 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009171#file1009171line235> > > > > 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. Have made all exceptions checked. > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java, line 57 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009172#file1009172line57> > > > > Let's create a new subclass of querycost. > > > > That way `Addable` won't be in `add` signature Have added a new impl. > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java, line 76 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009172#file1009172line76> > > > > 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. I am throwing UnsupportedOperationException as its currently not supported in ES > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java, lines 197-205 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009172#file1009172line197> > > > > What do empty implementation mean? Is it okay to do nothing? ESClient does not maintain any active connections to the server. So no closing required. Added comments for the same. > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ESDriver.java, lines 260-267 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009172#file1009172line260> > > > > Let's at least implement these if we want persistence on restart. Is it not possible to abstract this across all drivers? If yes I can try taking this up instead of coding it specifically for ES > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/ESDriverConfig.java, line 56 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009173#file1009173line56> > > > > Why do we need a constant for India specific time zone? Removed. > On July 13, 2015, 7:40 a.m., Rajat Khandelwal wrote: > > lens-driver-es/src/main/java/org/apache/lens/es/client/jest/JestClientImpl.java, > > line 101 > > <https://reviews.apache.org/r/36434/diff/1/?file=1009177#file1009177line101> > > > > I think we need to create a private default constructor here to avoid > > checkstyle errors. I have compiled with checkstyles. Did not get any errors - Amruth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36434/#review91435 ----------------------------------------------------------- On July 13, 2015, 4:19 p.m., Amruth Sampath wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36434/ > ----------------------------------------------------------- > > (Updated July 13, 2015, 4:19 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 > >
