hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "cheng xu" <cheng.a...@intel.com>
Subject Re: Review Request 26968: HIVE-8122: convert ExprNode to Parquet supported FilterPredict
Date Sun, 16 Nov 2014 03:50:52 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message