You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Ashutosh Chauhan <ha...@apache.org> on 2016/04/04 20:52:47 UTC
Review Request 45697: HIVE-13380 : Decimal should have lower
precedence than Double/Float
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45697/
-----------------------------------------------------------
Review request for hive and Jason Dere.
Bugs: HIVE-13380
https://issues.apache.org/jira/browse/HIVE-13380
Repository: hive-git
Description
-------
Additionally, string should not be allowed to implictly cast to decimal since there can be loss of precision there.
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/udf/UDFSign.java 022b130
ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java 6a83c32
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPMinus.java 771a6c7
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPMultiply.java 696682f
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPPlus.java eba4894
ql/src/test/queries/clientpositive/alter_partition_change_col.q 6861ca2
ql/src/test/queries/clientpositive/alter_table_cascade.q 479fda4
ql/src/test/results/clientpositive/perf/query32.q.out f9cfd69
ql/src/test/results/clientpositive/perf/query65.q.out 37bb1b3
ql/src/test/results/clientpositive/perf/query75.q.out 25a8776
ql/src/test/results/clientpositive/perf/query89.q.out 0cda449
ql/src/test/results/clientpositive/tez/vector_decimal_expressions.q.out 2976cb5
ql/src/test/results/clientpositive/udf_greatest.q.out 47cfb3f
ql/src/test/results/clientpositive/udf_least.q.out 2363abe
ql/src/test/results/clientpositive/vector_decimal_expressions.q.out 3ca326d
serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoUtils.java d3bb4e4
Diff: https://reviews.apache.org/r/45697/diff/
Testing
-------
Regression suite
Thanks,
Ashutosh Chauhan
Re: Review Request 45697: HIVE-13380 : Decimal should have lower
precedence than Double/Float
Posted by Ashutosh Chauhan <ha...@apache.org>.
> On April 10, 2016, 10:12 p.m., Jason Dere wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java, line 92
> > <https://reviews.apache.org/r/45697/diff/2/?file=1325411#file1325411line92>
> >
> > Since constant floating point values are in double, are there any big behavior changes here?
> >
> > - Comparisons (for example decimal_col = 12.5): I guess this means the comparison is done as double. I suspect this will affect ORC PPD because the column would need to be converted to double. Unless the constant value is converted to decimal constant (12.5BD). This could also be fixed by changing the Hive constant rules so that floating point constants are treated as Decimal types rather than double.
> >
> > - Does this affect IN clauses? I guess it's similar to comparison in that the comparison will now be done as doubles.
> >
> > - Table inserts from double into a decimal column will have an explicity cast to decimal, so that is ok.
> >
> > - I guess if there is an old-style UDF that only has a Decimal implementation, we would probably want to have a double version as well. I don't know if any such UDFs actually exist, if there was only one version of the UDF they usually have Double args.
> >
> > - Are there any other cases to think about that get affected by this change?
* yeah you are right since comparison will happen in double, casts need to be placed on decimal_col. However, for expression like double_col = 12.5BD after the patch there will be no implicit cast on column, so we will enable those patterns without cast.
* Yeah for IN also if both double & decimal are present, than comparison will happen in double. However, I noticed that In uses getCommonClass() instead of getCommonClassForComparison(). That needs to be corrected. Also GenericUDFUtils::update() has a hack involving decimal/double type which should no longer be necessary adter this change.
* Correct, inserts into decimal from double will need explicit cast.
* One such I came across was UDFSign, there might be more.
* This is all I can think of.
- Ashutosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45697/#review128049
-----------------------------------------------------------
On April 5, 2016, 1:23 a.m., Ashutosh Chauhan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45697/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 1:23 a.m.)
>
>
> Review request for hive and Jason Dere.
>
>
> Bugs: HIVE-13380
> https://issues.apache.org/jira/browse/HIVE-13380
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Additionally, string should not be allowed to implictly cast to decimal since there can be loss of precision there.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/UDFSign.java 022b130
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java 8488c21
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPMinus.java 771a6c7
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPMultiply.java 696682f
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPPlus.java eba4894
> ql/src/test/queries/clientpositive/alter_partition_change_col.q 6861ca2
> ql/src/test/queries/clientpositive/alter_table_cascade.q 479fda4
> ql/src/test/results/clientpositive/perf/query32.q.out f9cfd69
> ql/src/test/results/clientpositive/perf/query65.q.out 37bb1b3
> ql/src/test/results/clientpositive/perf/query75.q.out 25a8776
> ql/src/test/results/clientpositive/perf/query89.q.out 0cda449
> ql/src/test/results/clientpositive/tez/vector_decimal_expressions.q.out 2976cb5
> ql/src/test/results/clientpositive/udf_greatest.q.out 47cfb3f
> ql/src/test/results/clientpositive/udf_least.q.out 2363abe
> ql/src/test/results/clientpositive/vector_decimal_expressions.q.out 3ca326d
> serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoUtils.java d3bb4e4
>
> Diff: https://reviews.apache.org/r/45697/diff/
>
>
> Testing
> -------
>
> Regression suite
>
>
> Thanks,
>
> Ashutosh Chauhan
>
>
Re: Review Request 45697: HIVE-13380 : Decimal should have lower
precedence than Double/Float
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45697/#review128049
-----------------------------------------------------------
ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java (line 90)
<https://reviews.apache.org/r/45697/#comment191442>
Since constant floating point values are in double, are there any big behavior changes here?
- Comparisons (for example decimal_col = 12.5): I guess this means the comparison is done as double. I suspect this will affect ORC PPD because the column would need to be converted to double. Unless the constant value is converted to decimal constant (12.5BD). This could also be fixed by changing the Hive constant rules so that floating point constants are treated as Decimal types rather than double.
- Does this affect IN clauses? I guess it's similar to comparison in that the comparison will now be done as doubles.
- Table inserts from double into a decimal column will have an explicity cast to decimal, so that is ok.
- I guess if there is an old-style UDF that only has a Decimal implementation, we would probably want to have a double version as well. I don't know if any such UDFs actually exist, if there was only one version of the UDF they usually have Double args.
- Are there any other cases to think about that get affected by this change?
- Jason Dere
On April 5, 2016, 1:23 a.m., Ashutosh Chauhan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45697/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 1:23 a.m.)
>
>
> Review request for hive and Jason Dere.
>
>
> Bugs: HIVE-13380
> https://issues.apache.org/jira/browse/HIVE-13380
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Additionally, string should not be allowed to implictly cast to decimal since there can be loss of precision there.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/UDFSign.java 022b130
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java 8488c21
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPMinus.java 771a6c7
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPMultiply.java 696682f
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPPlus.java eba4894
> ql/src/test/queries/clientpositive/alter_partition_change_col.q 6861ca2
> ql/src/test/queries/clientpositive/alter_table_cascade.q 479fda4
> ql/src/test/results/clientpositive/perf/query32.q.out f9cfd69
> ql/src/test/results/clientpositive/perf/query65.q.out 37bb1b3
> ql/src/test/results/clientpositive/perf/query75.q.out 25a8776
> ql/src/test/results/clientpositive/perf/query89.q.out 0cda449
> ql/src/test/results/clientpositive/tez/vector_decimal_expressions.q.out 2976cb5
> ql/src/test/results/clientpositive/udf_greatest.q.out 47cfb3f
> ql/src/test/results/clientpositive/udf_least.q.out 2363abe
> ql/src/test/results/clientpositive/vector_decimal_expressions.q.out 3ca326d
> serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoUtils.java d3bb4e4
>
> Diff: https://reviews.apache.org/r/45697/diff/
>
>
> Testing
> -------
>
> Regression suite
>
>
> Thanks,
>
> Ashutosh Chauhan
>
>
Re: Review Request 45697: HIVE-13380 : Decimal should have lower
precedence than Double/Float
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45697/
-----------------------------------------------------------
(Updated April 5, 2016, 1:23 a.m.)
Review request for hive and Jason Dere.
Changes
-------
Rebased
Bugs: HIVE-13380
https://issues.apache.org/jira/browse/HIVE-13380
Repository: hive-git
Description
-------
Additionally, string should not be allowed to implictly cast to decimal since there can be loss of precision there.
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/udf/UDFSign.java 022b130
ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java 8488c21
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPMinus.java 771a6c7
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPMultiply.java 696682f
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFOPPlus.java eba4894
ql/src/test/queries/clientpositive/alter_partition_change_col.q 6861ca2
ql/src/test/queries/clientpositive/alter_table_cascade.q 479fda4
ql/src/test/results/clientpositive/perf/query32.q.out f9cfd69
ql/src/test/results/clientpositive/perf/query65.q.out 37bb1b3
ql/src/test/results/clientpositive/perf/query75.q.out 25a8776
ql/src/test/results/clientpositive/perf/query89.q.out 0cda449
ql/src/test/results/clientpositive/tez/vector_decimal_expressions.q.out 2976cb5
ql/src/test/results/clientpositive/udf_greatest.q.out 47cfb3f
ql/src/test/results/clientpositive/udf_least.q.out 2363abe
ql/src/test/results/clientpositive/vector_decimal_expressions.q.out 3ca326d
serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoUtils.java d3bb4e4
Diff: https://reviews.apache.org/r/45697/diff/
Testing
-------
Regression suite
Thanks,
Ashutosh Chauhan