> On Feb. 13, 2018, 12:50 a.m., Vihang Karajgaonkar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedPrimitiveColumnReader.java
> > Lines 253-254 (original), 255-256 (patched)
> > <https://reviews.apache.org/r/65478/diff/4/?file=1957004#file1957004line261>
> >
> >     before the patch, the code assumes that the ColumnReader's type matches 
> > with what the hive's type is. So these two lines made sense. But after the 
> > patch, it is possible that the underlying parquet type does not have 
> > decimalMetadata so it these lines will throw a NPE. Instead of throwing 
> > NPE, we should check if decimalMetadata is set and throw IOException here 
> > saying that underlying type cannot be converted to decimal. What do you 
> > think?
> >     
> >     Also, may be we should add a TODO here to use default precision and 
> > scale if its not available for the future.
> 
> cheng xu wrote:
>     Why do we need the default precision and scale here? We can hardly read 
> data as decimal if those values are missing.

hmm .. I am not a 100% sure but I thought HiveDecimal defines something like 
USER_DEFAULT_PRECISION and USER_DEFAULT_SCALE which is used when user doesn't 
provide scale and precision. This is not important for this change so you can 
ignore the comment related to default precision and scale. But I think we 
should protect against a NPE which will be thrown if the user tries to change 
say an int column to a decimal(10,2) (I know it doesn't work anyways, but I 
think it would be better to throw an exception rather than failing with a NPE)


- Vihang


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65478/#review197313
-----------------------------------------------------------


On Feb. 12, 2018, 8:25 a.m., cheng xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65478/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2018, 8:25 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> VectorizedParquetReader throws an exception when trying to reading from a 
> parquet table on which new columns are added.
> 
> 
> Diffs
> -----
> 
>   
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/BaseVectorizedColumnReader.java
>  907a9b8 
>   
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/ParquetDataColumnReader.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/ParquetDataColumnReaderFactory.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedDummyColumnReader.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedListColumnReader.java
>  c36640d 
>   
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedParquetRecordReader.java
>  08ac57b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedPrimitiveColumnReader.java
>  39689f1 
>   
> ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestVectorizedColumnReader.java
>  9e414dc 
>   
> ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestVectorizedDictionaryEncodingColumnReader.java
>  3e5d831 
>   
> ql/src/test/org/apache/hadoop/hive/ql/io/parquet/VectorizedColumnReaderTestBase.java
>  5d3ebd6 
>   
> ql/src/test/queries/clientpositive/schema_evol_par_vec_table_dictionary_encoding.q
>  PRE-CREATION 
>   
> ql/src/test/queries/clientpositive/schema_evol_par_vec_table_non_dictionary_encoding.q
>  PRE-CREATION 
>   ql/src/test/results/clientpositive/schema_evol_par_vec_table.q.out 
> PRE-CREATION 
>   
> ql/src/test/results/clientpositive/schema_evol_par_vec_table_dictionary_encoding.q.out
>  PRE-CREATION 
>   
> ql/src/test/results/clientpositive/schema_evol_par_vec_table_non_dictionary_encoding.q.out
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65478/diff/4/
> 
> 
> Testing
> -------
> 
> Newly added UT passed and qtest passed locally.
> 
> 
> Thanks,
> 
> cheng xu
> 
>

Reply via email to