----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26968/#review61668 -----------------------------------------------------------
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. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/FilterPredicateLeafBuilder.java <https://reviews.apache.org/r/26968/#comment103427> please move import up (alphabetical order) ql/src/java/org/apache/hadoop/hive/ql/io/parquet/FilterPredicateLeafBuilder.java <https://reviews.apache.org/r/26968/#comment103429> For consistency, could we call it "literals" instead of "constants" ? (since this argument come from PredicateLeaf, where it's called a literal). Calling it by a different name here is confusing. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/FilterPredicateLeafBuilder.java <https://reviews.apache.org/r/26968/#comment103430> for (Object literal : literals) ql/src/java/org/apache/hadoop/hive/ql/io/parquet/FilterPredicateLeafBuilder.java <https://reviews.apache.org/r/26968/#comment103432> This is pretty serious, right ? Should be throwing a RuntimeException here. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/FilterPredicateLeafBuilder.java <https://reviews.apache.org/r/26968/#comment103433> instead throw new RuntimeException("Unknown PredicateLeaf Operator type: " + ... ) ql/src/java/org/apache/hadoop/hive/ql/io/parquet/FilterPredicateLeafBuilder.java <https://reviews.apache.org/r/26968/#comment103437> build -> Build ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103438> please alphabetize ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103439> move this below as part of param description ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103440> "literal" ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103441> same as above; throw expection ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103442> same as above; throw expection ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103443> same as above; throw expection ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103444> same as above; throw expection ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103445> same as above; throw expection ql/src/java/org/apache/hadoop/hive/ql/io/parquet/LeafFilterFactory.java <https://reviews.apache.org/r/26968/#comment103446> Good to log debug statement here: LOG.debug("Conversion to Parquet FilterPredicate not supported for " + type); ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java <https://reviews.apache.org/r/26968/#comment103450> I assume we are checking READ_COLUMN_NAMES_CONF_STR because this is set only when we do predicate pushdown. Correct ? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java <https://reviews.apache.org/r/26968/#comment103448> move to one line ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java <https://reviews.apache.org/r/26968/#comment103449> combine into one line ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment103452> @Override ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment103453> @Override ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment103454> nit: "File format " + format + " is not supported ... ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment103455> To avoid duplicating it for getOrcLiteral(), let's move this code to a separate method ? ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment103458> Could you add name of exception in the log line ? LOG.error("Failed to build predicate filter leaf with errors " + e, e); ql/src/test/org/apache/hadoop/hive/ql/io/sarg/TestSearchArgumentImpl.java <https://reviews.apache.org/r/26968/#comment103460> Should we add one for date, decimal and timestamp ? -- ones we don't support currently -- just to make sure we're not blowing up. - Mohit Sabharwal 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 > >
