----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26968/#review61267 -----------------------------------------------------------
Had a few high level questions. Will take another pass after that. I'd appreciate adding more comments to the code. pom.xml <https://reviews.apache.org/r/26968/#comment102912> I think we need an actual release version (not a release candidate) ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java <https://reviews.apache.org/r/26968/#comment102920> conditional not needed. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java <https://reviews.apache.org/r/26968/#comment102921> log error is filterpredicate is null ? ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102799> please move these to the top. (imports need to be in alphabetized order.) ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102898> Could you explain why we need this? Wasn't this step already happening in boxLiteral() ? ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102899> we should be able to get rid of this duplication by passing FileFormat to this method. ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102856> getOrcLiteralList() ? ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102900> Could you explain why this special handling is needed ? ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102905> should be or() instead of and() Please add unit test for to catch this case :) ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102906> pad else with spaces ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102909> These two lines are unrelated to the conditional. Could you move these out ? ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102922> Don't we still need to return a Long (and not an Integer) for ORC ? Also, why not handle Long case here as well ? ql/src/test/org/apache/hadoop/hive/ql/io/sarg/TestSearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102794> Please add assert for leaf.getParquetType() everywhere we are checking leaf.getOrcType() ql/src/test/org/apache/hadoop/hive/ql/io/sarg/TestSearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment102795> Please add assert for leaf.getParquetLiteral() everywhere we are checking leaf.getOrcLiteral() serde/src/java/org/apache/hadoop/hive/ql/io/sarg/PredicateLeaf.java <https://reviews.apache.org/r/26968/#comment102791> parquet -> ORC serde/src/java/org/apache/hadoop/hive/ql/io/sarg/PredicateLeaf.java <https://reviews.apache.org/r/26968/#comment102815> Did you consider adding another enum to this interface like enum FileFormat { ORC, PARQUET } instead of adding getOrc and getParquet methods ? We can pass FileFormat as an arg to the get methods, i.e: getLiteral(FileFormat) getType(FileFormat) getLiteralList(FileFormat) IMO, that's extensible and avoids code duplication, for example between getOrcLiteral() and getParquetLiteral(). serde/src/java/org/apache/hadoop/hive/ql/io/sarg/PredicateLeaf.java <https://reviews.apache.org/r/26968/#comment102792> ORC -> parquet - Mohit Sabharwal On Oct. 21, 2014, 8:13 a.m., cheng xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26968/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 8:13 a.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > HIVE-8122: convert ExprNode to Parquet supported FilterPredict > > > Diffs > ----- > > pom.xml a5f851f31df15660cebef0e4691ea34699c6d1ef > ql/src/java/org/apache/hadoop/hive/ql/io/orc/RecordReaderImpl.java > f8fa316b8da29f8970ed6839c7c2f883fa8d9829 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/FilterPredicateLeafBuilder.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java > f5da46d392d8ac5f5589f66c37d567b1d3bd8843 > ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java > eeb9641545ed0ad69f3bbc9a8383697fc7efe37d > ql/src/test/org/apache/hadoop/hive/ql/io/sarg/TestSearchArgumentImpl.java > 831ef8c8ec64c270ef62d5336b4cc78d9e34b398 > serde/pom.xml 98e55061b6b3abe18030b0b8d3f511bd98bee5f7 > serde/src/java/org/apache/hadoop/hive/ql/io/sarg/PredicateLeaf.java > 616c6dbd1ec71ad178f41e8666bad2500e68e151 > serde/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgument.java > db0f0148e2a995534a4c1369fc4c542cd0b4e6ab > > Diff: https://reviews.apache.org/r/26968/diff/ > > > Testing > ------- > > local UT passed > > > Thanks, > > cheng xu > >
