> On April 22, 2016, 8:09 a.m., Rajat Khandelwal wrote: > > lens-client/src/main/java/org/apache/lens/client/exceptions/LensClientIOException.java, > > line 22 > > <https://reviews.apache.org/r/46485/diff/1/?file=1354877#file1354877line22> > > > > should it be `extends IOException`?
Good Suggestion . Will do that . Checked , most places IOException is being thrown as LensClientIOExecption, execpt one place where we are calling LensStatement#getHttpResultSet(org.apache.lens.api.query.LensQuery). Here a run time execption is being thrown. Will catch this in lens client and throw LensClientIOExecption to not hassle the user with handling mutiple exceptions. > On April 22, 2016, 8:09 a.m., Rajat Khandelwal wrote: > > lens-client/src/main/java/org/apache/lens/client/resultset/AbstractResultSet.java, > > line 94 > > <https://reviews.apache.org/r/46485/diff/1/?file=1354878#file1354878line94> > > > > Let's make the class implement `Closeable`. > > > > Secondly, it seems like `throws` is redundant. - The execpetion is kept so that the subcalsses can thorw execption if required. Thought, as of now none of the subcalsses are overriding this method . - Not implementing Closeable as this is more of an internal method (will not be available publically) > On April 22, 2016, 8:09 a.m., Rajat Khandelwal wrote: > > lens-client/src/main/java/org/apache/lens/client/resultset/CsvResultSetReader.java, > > lines 43-45 > > <https://reviews.apache.org/r/46485/diff/1/?file=1354880#file1354880line43> > > > > Multiple `getNext` calls will return same line unless combined with > > `hasNext`. This is counter intuitive while designing an iterator. Refer > > http://stackoverflow.com/a/29061917 Was trying to mimic jdbc result set where the hasNext() (actually its called boolean next() which makes more sense ) moves the cursor by one position. Will change method names boolean hasNext() = booelan next(); String[] next() = String[] getRow()/getCurrentRow(); The users should use it like JDBC ResultSet. This should remove the confusion around mutiple ways to access it. > On April 22, 2016, 8:09 a.m., Rajat Khandelwal wrote: > > lens-client/src/main/java/org/apache/lens/client/resultset/ResultSet.java, > > line 29 > > <https://reviews.apache.org/r/46485/diff/1/?file=1354881#file1354881line29> > > > > I think this can also implement `Iterator<String[]>` which will imply > > that the syntax like the following is supported: > > > > ``` > > ResultSet rs = ...; > > for(String[] line: rs) { > > ... > > } > > ``` The Iterator syntax which you suggested looks tempting. Looked at java.util.Iterator . It has more methods which will not make sense (remove, forEachRemaining). More methods can be added in future too. Want to stick to booelan next() and String[] getRow() and not expose other dummy methods from iterator. > On April 22, 2016, 8:09 a.m., Rajat Khandelwal wrote: > > lens-client/src/main/java/org/apache/lens/client/resultset/ZippedCsvResultSet.java, > > line 35 > > <https://reviews.apache.org/r/46485/diff/1/?file=1354883#file1354883line35> > > > > Return type here can be `ZippedCsvResultSetReader` so that someone > > testing `ZippedCsvResultSet` can directly expect that type when calling > > `createResultSetReader` without having to type-cast. As of now all test cases are coded to interface and not specific implemenatations. Also having the interface as reutn type will give subclasses an option to return a different implemenattion type in future if required. Don't see a pressing need to expose actual type here. - Puneet ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46485/#review130059 ----------------------------------------------------------- On April 21, 2016, 11:42 a.m., Puneet Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46485/ > ----------------------------------------------------------- > > (Updated April 21, 2016, 11:42 a.m.) > > > Review request for lens. > > > Bugs: LENS-1008 > https://issues.apache.org/jira/browse/LENS-1008 > > > Repository: lens > > > Description > ------- > > The result iterator can have the following methods > public interface ResultSet > { public boolean hasNext(); public String[] next(); public String[] > getColumnNames(); } > This can be used by clients to programmatically consume the result with ease. > > > Have supported Zipped CSV abd CSV for now with configurable delimiter, > encoding and isHeaderPresent. > It can be extend to other formats in future > > Users can access this feature via easily via (Another varaiation avialble > with more options) > LensClient.getCsvResultSet(QueryHandle) > LensClient.getZippedCsvResultSet(QueryHandle) > > > Diffs > ----- > > lens-client/pom.xml 4fd01fb > lens-client/src/main/java/org/apache/lens/client/LensClient.java 9626820 > > lens-client/src/main/java/org/apache/lens/client/exceptions/LensClientIOException.java > PRE-CREATION > > lens-client/src/main/java/org/apache/lens/client/resultset/AbstractResultSet.java > PRE-CREATION > > lens-client/src/main/java/org/apache/lens/client/resultset/CsvResultSet.java > PRE-CREATION > > lens-client/src/main/java/org/apache/lens/client/resultset/CsvResultSetReader.java > PRE-CREATION > lens-client/src/main/java/org/apache/lens/client/resultset/ResultSet.java > PRE-CREATION > > lens-client/src/main/java/org/apache/lens/client/resultset/ResultSetReader.java > PRE-CREATION > > lens-client/src/main/java/org/apache/lens/client/resultset/ZippedCsvResultSet.java > PRE-CREATION > > lens-client/src/main/java/org/apache/lens/client/resultset/ZippedCsvResultSetReader.java > PRE-CREATION > > lens-client/src/main/java/org/apache/lens/client/resultset/ZippedResultSetReader.java > PRE-CREATION > lens-client/src/test/java/org/apache/lens/client/TestLensClient.java > 7a00f65 > lens-client/src/test/resources/dim2-part/data.data PRE-CREATION > lens-client/src/test/resources/dim_table.xml PRE-CREATION > lens-client/src/test/resources/fact1.xml PRE-CREATION > lens-client/src/test/resources/lens-client-site.xml b356e5e > lens-client/src/test/resources/local-storage.xml PRE-CREATION > lens-client/src/test/resources/sample-cube.xml PRE-CREATION > lens-client/src/test/resources/test-detail.xml PRE-CREATION > lens-client/src/test/resources/test-dimension.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/46485/diff/ > > > Testing > ------- > > ------------------------------------------------------- > T E S T S > ------------------------------------------------------- > Listening for transport dt_socket at address: 8000 > Running org.apache.lens.client.TestLensClient > Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 145.247 sec - > in org.apache.lens.client.TestLensClient > > Results : > > Tests run: 7, Failures: 0, Errors: 0, Skipped: 0 > > > Thanks, > > Puneet Gupta > >
