Re: Review Request 70920: HIVE-21868: Vectorize CAST...FORMAT

2019-07-09 Thread Marta Kuczora via Review Board 

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


Ship it!




Ship It!

- Marta Kuczora


On July 4, 2019, 3:04 p.m., Karen Coppage wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70920/
> ---
> 
> (Updated July 4, 2019, 3:04 p.m.)
> 
> 
> Review request for hive and Marta Kuczora.
> 
> 
> Bugs: HIVE-21868
> https://issues.apache.org/jira/browse/HIVE-21868
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Vectorize UDFs for CAST ( AS STRING/CHAR/VARCHAR FORMAT 
> ) and CAST ( AS TIMESTAMP/DATE FORMAT ).
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/format/datetime/HiveSqlDateTimeFormatter.java
>  4e024a357b 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 
> fa9d1e9783 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToString.java
>  dfa9f8a00d 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToStringWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToVarCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDate.java
>  a6dff12e1a 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDateWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToTimestamp.java
>  b48b0136eb 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToTimestampWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToString.java
>  adc3a9d7b9 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToStringWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToVarCharWithFormat.java
>  PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCastFormat.java 
> 16742eee9b 
>   
> ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorMathFunctions.java
>  663237739e 
>   
> ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorTypeCasts.java
>  58fd7b030e 
>   
> ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorTypeCastsWithFormat.java
>  PRE-CREATION 
>   ql/src/test/queries/clientnegative/udf_cast_format_bad_pattern.q 
> PRE-CREATION 
>   ql/src/test/queries/clientpositive/cast_datetime_with_sql_2016_format.q 
> 269edf6da6 
>   ql/src/test/results/clientnegative/udf_cast_format_bad_pattern.q.out 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/cast_datetime_with_sql_2016_format.q.out 
> 4a502b9700 
> 
> 
> Diff: https://reviews.apache.org/r/70920/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Karen Coppage
> 
>

Re: Review Request 70920: HIVE-21868: Vectorize CAST...FORMAT

2019-07-04 Thread Karen Coppage via Review Board 

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

(Updated July 4, 2019, 3:04 p.m.)


Review request for hive and Marta Kuczora.


Changes
---

Patch 7


Bugs: HIVE-21868
https://issues.apache.org/jira/browse/HIVE-21868


Repository: hive-git


Description
---

Vectorize UDFs for CAST ( AS STRING/CHAR/VARCHAR FORMAT 
) and CAST ( AS TIMESTAMP/DATE FORMAT ).


Diffs (updated)
-

  
common/src/java/org/apache/hadoop/hive/common/format/datetime/HiveSqlDateTimeFormatter.java
 4e024a357b 
  ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 
fa9d1e9783 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToCharWithFormat.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToString.java
 dfa9f8a00d 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToStringWithFormat.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToVarCharWithFormat.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDate.java
 a6dff12e1a 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDateWithFormat.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToTimestamp.java
 b48b0136eb 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToTimestampWithFormat.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToCharWithFormat.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToString.java
 adc3a9d7b9 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToStringWithFormat.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToVarCharWithFormat.java
 PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCastFormat.java 
16742eee9b 
  
ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorMathFunctions.java
 663237739e 
  
ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorTypeCasts.java
 58fd7b030e 
  
ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorTypeCastsWithFormat.java
 PRE-CREATION 
  ql/src/test/queries/clientnegative/udf_cast_format_bad_pattern.q PRE-CREATION 
  ql/src/test/queries/clientpositive/cast_datetime_with_sql_2016_format.q 
269edf6da6 
  ql/src/test/results/clientnegative/udf_cast_format_bad_pattern.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/cast_datetime_with_sql_2016_format.q.out 
4a502b9700 


Diff: https://reviews.apache.org/r/70920/diff/4/

Changes: https://reviews.apache.org/r/70920/diff/3-4/


Testing
---


Thanks,

Karen Coppage

Re: Review Request 70920: HIVE-21868: Vectorize CAST...FORMAT

2019-07-03 Thread Karen Coppage via Review Board 


> On July 3, 2019, 10:51 a.m., Marta Kuczora wrote:
> > Thanks a lot Karen for the patch!
> > I have some questions, but otherwise the change looks good to me.

Thanks very much for the review!


> On July 3, 2019, 10:51 a.m., Marta Kuczora wrote:
> > common/src/java/org/apache/hadoop/hive/common/format/datetime/HiveSqlDateTimeFormatter.java
> > Line 223 (original), 224 (patched)
> > 
> >
> > Why did you change the type of this variable to ArrayList from List?

I thought it was necessary in order to make the class serializable but now I 
see it's not. I'll fix this.


> On July 3, 2019, 10:51 a.m., Marta Kuczora wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToString.java
> > Lines 59 (patched)
> > 
> >
> > Do the CastDateToString, CastDateToChar and CastDateToVarchar udfs use 
> > this method, or is this just a typo and the CastDateToStringWithFormat, ... 
> > udfs use this?

They do, all 3 classes eventually inherit from CastDateToString.


> On July 3, 2019, 10:51 a.m., Marta Kuczora wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCastFormat.java
> > Line 200 (original), 202 (patched)
> > 
> >
> > Is the formattedOutput variable never going to be null after this 
> > change? If there is a scenario where it can be null, it will cause problems 
> > when trying to cast it.

The output of format(Timestamp|Date) is a .toString() which I 
believe will never return null.


> On July 3, 2019, 10:51 a.m., Marta Kuczora wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCastFormat.java
> > Line 217 (original), 220 (patched)
> > 
> >
> > The same question about being null (previous comment) applies to the t 
> > and d variable as well.

This is not the case now but may be later on (if timestamp range of - 
is ever strictly enforced). I will include a null check here and above just in 
case.


- Karen


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


On June 26, 2019, 8:44 a.m., Karen Coppage wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70920/
> ---
> 
> (Updated June 26, 2019, 8:44 a.m.)
> 
> 
> Review request for hive and Marta Kuczora.
> 
> 
> Bugs: HIVE-21868
> https://issues.apache.org/jira/browse/HIVE-21868
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Vectorize UDFs for CAST ( AS STRING/CHAR/VARCHAR FORMAT 
> ) and CAST ( AS TIMESTAMP/DATE FORMAT ).
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/format/datetime/HiveSqlDateTimeFormatter.java
>  4e024a357b 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 
> fa9d1e9783 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToString.java
>  dfa9f8a00d 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToStringWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToVarCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDate.java
>  a6dff12e1a 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDateWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToTimestamp.java
>  b48b0136eb 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToTimestampWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToString.java
>  adc3a9d7b9 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToStringWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToVarCharWithFormat.java
>  PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCastFormat.java 
> 16742eee9b 
>   
> ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorMathFunctions.java
>  663237739e 
>   
>

Re: Review Request 70920: HIVE-21868: Vectorize CAST...FORMAT

2019-07-03 Thread Marta Kuczora via Review Board 

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



Thanks a lot Karen for the patch!
I have some questions, but otherwise the change looks good to me.


common/src/java/org/apache/hadoop/hive/common/format/datetime/HiveSqlDateTimeFormatter.java
Line 223 (original), 224 (patched)


Why did you change the type of this variable to ArrayList from List?



ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToString.java
Lines 59 (patched)


Do the CastDateToString, CastDateToChar and CastDateToVarchar udfs use this 
method, or is this just a typo and the CastDateToStringWithFormat, ... udfs use 
this?



ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCastFormat.java
Line 200 (original), 202 (patched)


Is the formattedOutput variable never going to be null after this change? 
If there is a scenario where it can be null, it will cause problems when trying 
to cast it.



ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCastFormat.java
Line 217 (original), 220 (patched)


The same question about being null (previous comment) applies to the t and 
d variable as well.


- Marta Kuczora


On June 26, 2019, 8:44 a.m., Karen Coppage wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70920/
> ---
> 
> (Updated June 26, 2019, 8:44 a.m.)
> 
> 
> Review request for hive and Marta Kuczora.
> 
> 
> Bugs: HIVE-21868
> https://issues.apache.org/jira/browse/HIVE-21868
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Vectorize UDFs for CAST ( AS STRING/CHAR/VARCHAR FORMAT 
> ) and CAST ( AS TIMESTAMP/DATE FORMAT ).
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/format/datetime/HiveSqlDateTimeFormatter.java
>  4e024a357b 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 
> fa9d1e9783 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToString.java
>  dfa9f8a00d 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToStringWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToVarCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDate.java
>  a6dff12e1a 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDateWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToTimestamp.java
>  b48b0136eb 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToTimestampWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToCharWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToString.java
>  adc3a9d7b9 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToStringWithFormat.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToVarCharWithFormat.java
>  PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCastFormat.java 
> 16742eee9b 
>   
> ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorMathFunctions.java
>  663237739e 
>   
> ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorTypeCasts.java
>  58fd7b030e 
>   
> ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorTypeCastsWithFormat.java
>  PRE-CREATION 
>   ql/src/test/queries/clientnegative/udf_cast_format_bad_pattern.q 
> PRE-CREATION 
>   ql/src/test/queries/clientpositive/cast_datetime_with_sql_2016_format.q 
> 269edf6da6 
>   ql/src/test/results/clientnegative/udf_cast_format_bad_pattern.q.out 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/cast_datetime_with_sql_2016_format.q.out 
> 4a502b9700 
> 
> 
> Diff: https://reviews.apache.org/r/70920/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Karen Coppage
> 
>