> On Nov. 14, 2014, 12:33 a.m., Mohit Sabharwal wrote: > > Had a few high level questions. Will take another pass after that. I'd > > appreciate adding more comments to the code.
Thanks for your review. I added some inline comments below and update the patch. > On Nov. 14, 2014, 12:33 a.m., Mohit Sabharwal wrote: > > pom.xml, line 149 > > <https://reviews.apache.org/r/26968/diff/4/?file=751463#file751463line149> > > > > I think we need an actual release version (not a release candidate) At this point, I only found this RC version which has the filter feature supported. And since the parquet is close to its latest releas, we can change it later. > On Nov. 14, 2014, 12:33 a.m., Mohit Sabharwal wrote: > > ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java, > > lines 132-135 > > <https://reviews.apache.org/r/26968/diff/4/?file=751468#file751468line132> > > > > Could you explain why we need this? > > > > Wasn't this step already happening in boxLiteral() ? I look at the code again and find it has been done in getType(ExprNodeDesc expr) method of ExpressionBuilder class. Thanks for pointing out this! > On Nov. 14, 2014, 12:33 a.m., Mohit Sabharwal wrote: > > ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java, > > lines 153-155 > > <https://reviews.apache.org/r/26968/diff/4/?file=751468#file751468line153> > > > > Could you explain why this special handling is needed ? Only Integer type needs to be cast into Long type when trying to get the literal list. > On Nov. 14, 2014, 12:33 a.m., Mohit Sabharwal wrote: > > ql/src/java/org/apache/hadoop/hive/ql/io/sarg/SearchArgumentImpl.java, line > > 1099 > > <https://reviews.apache.org/r/26968/diff/4/?file=751468#file751468line1099> > > > > Don't we still need to return a Long (and not an Integer) for ORC ? > > > > Also, why not handle Long case here as well ? This method is used by orc file which needs Long type. > On Nov. 14, 2014, 12:33 a.m., Mohit Sabharwal wrote: > > ql/src/test/org/apache/hadoop/hive/ql/io/sarg/TestSearchArgumentImpl.java, > > line 765 > > <https://reviews.apache.org/r/26968/diff/4/?file=751469#file751469line765> > > > > Please add assert for leaf.getParquetType() everywhere we are checking > > leaf.getOrcType() No need anymore since reserve the previous implement for getType(). - cheng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26968/#review61267 ----------------------------------------------------------- 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 > >
