> On Nov. 16, 2014, 12:49 a.m., Mohit Sabharwal wrote: > > Thank you for making the changes! I had a few more comments on error > > handling and readability. But otherwise, looks good to me. Thanks! > > > > Could you please add import statements for all the inner classes? This > > makes things more readable. For example: > > import org.apache.hadoop.hive.ql.io.sarg.PredicateLeaf.FileFormat > > import static parquet.filter2.predicate.FilterApi.eq > > import static parquet.filter2.predicate.FilterApi.intColumn > > and so on... > > If you're using Intellij, there is an option to "Insert imports for inner > > classes". I'm sure there is one in Eclipse as well.
Thank you for your conscientious review. Update the patch according to your comments. > On Nov. 16, 2014, 12:49 a.m., Mohit Sabharwal wrote: > > ql/src/test/org/apache/hadoop/hive/ql/io/sarg/TestSearchArgumentImpl.java, > > lines 1710-1715 > > <https://reviews.apache.org/r/26968/diff/6/?file=764696#file764696line1710> > > > > Should we add one for date, decimal and timestamp ? -- ones we don't > > support currently -- just to make sure we're not blowing up. Thanks for pointing this out. The test cases named in testBuilderComplexTypes* covered it. > On Nov. 16, 2014, 12:49 a.m., Mohit Sabharwal wrote: > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java, > > lines 125-129 > > <https://reviews.apache.org/r/26968/diff/6/?file=764694#file764694line125> > > > > I assume we are checking READ_COLUMN_NAMES_CONF_STR because this is set > > only when we do predicate pushdown. Correct ? Yes, that's right! - cheng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26968/#review61668 ----------------------------------------------------------- On Nov. 15, 2014, 2:51 a.m., cheng xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26968/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2014, 2:51 a.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > HIVE-8122: convert ExprNode to Parquet supported FilterPredict > > > Diffs > ----- > > pom.xml 793ea86 > ql/src/java/org/apache/hadoop/hive/ql/io/orc/RecordReaderImpl.java a6a0ec1 > > 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 > f5da46d > ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java > eeb9641 > ql/src/test/org/apache/hadoop/hive/ql/io/sarg/TestSearchArgumentImpl.java > c91644c > serde/pom.xml 98e5506 > serde/src/java/org/apache/hadoop/hive/ql/io/sarg/PredicateLeaf.java 616c6db > serde/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgument.java > db0f014 > > Diff: https://reviews.apache.org/r/26968/diff/ > > > Testing > ------- > > local UT passed > > > Thanks, > > cheng xu > >
