You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Xuefu Zhang <xz...@cloudera.com> on 2013/11/05 00:40:09 UTC

Review Request 15217: HIVE-5726: The DecimalTypeInfo instance associated with a decimal constant is not in line with the precision/scale of the constant

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

Review request for hive.


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


Repository: hive-git


Description
-------

With this patch, the resuting schema from constant is in accordance with the constant instead of the default from the type.


Diffs
-----

  common/src/java/org/apache/hadoop/hive/common/type/HiveDecimal.java 606f6d4 
  ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java 47d7995 
  ql/src/test/queries/clientpositive/ctas_hadoop20.q 4961b97 
  ql/src/test/results/clientpositive/ctas_hadoop20.q.out 885e985 
  ql/src/test/results/clientpositive/literal_decimal.q.out 2d1ae55 

Diff: https://reviews.apache.org/r/15217/diff/


Testing
-------

New tests are added. Test suite passed.


Thanks,

Xuefu Zhang

Re: Review Request 15217: HIVE-5726: The DecimalTypeInfo instance associated with a decimal constant is not in line with the precision/scale of the constant

Posted by Xuefu Zhang <xz...@cloudera.com>.

> On Nov. 5, 2013, 7:31 p.m., Brock Noland wrote:
> > This looks very good!  Why didn't we add a new test to TestHiveDecimal? It looks like this functionality could be tested there.

Were you referring to the change made to HiveDecimal? If so, yes, that can be tested in TestHiveDecimal. However, the main objection of the the patch cannot be tested in TestHiveDecimal, as HiveDecimal represents data only, but type info is about metadata.


- Xuefu


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


On Nov. 4, 2013, 11:40 p.m., Xuefu Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15217/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2013, 11:40 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-5726
>     https://issues.apache.org/jira/browse/HIVE-5726
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> With this patch, the resuting schema from constant is in accordance with the constant instead of the default from the type.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/common/type/HiveDecimal.java 606f6d4 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java 47d7995 
>   ql/src/test/queries/clientpositive/ctas_hadoop20.q 4961b97 
>   ql/src/test/results/clientpositive/ctas_hadoop20.q.out 885e985 
>   ql/src/test/results/clientpositive/literal_decimal.q.out 2d1ae55 
> 
> Diff: https://reviews.apache.org/r/15217/diff/
> 
> 
> Testing
> -------
> 
> New tests are added. Test suite passed.
> 
> 
> Thanks,
> 
> Xuefu Zhang
> 
>

Re: Review Request 15217: HIVE-5726: The DecimalTypeInfo instance associated with a decimal constant is not in line with the precision/scale of the constant

Posted by Brock Noland <br...@cloudera.com>.

> On Nov. 5, 2013, 7:31 p.m., Brock Noland wrote:
> > This looks very good!  Why didn't we add a new test to TestHiveDecimal? It looks like this functionality could be tested there.
> 
> Xuefu Zhang wrote:
>     Were you referring to the change made to HiveDecimal? If so, yes, that can be tested in TestHiveDecimal. However, the main objection of the the patch cannot be tested in TestHiveDecimal, as HiveDecimal represents data only, but type info is about metadata.

Sorry I wasn't clear. Yes I am asking why we didn't add new test functionality to TestHiveDecimal for the changes in HiveDecimal. The change looks like it's a good candidate for a unit test to ensure we don't regress.


- Brock


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


On Nov. 4, 2013, 11:40 p.m., Xuefu Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15217/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2013, 11:40 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-5726
>     https://issues.apache.org/jira/browse/HIVE-5726
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> With this patch, the resuting schema from constant is in accordance with the constant instead of the default from the type.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/common/type/HiveDecimal.java 606f6d4 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java 47d7995 
>   ql/src/test/queries/clientpositive/ctas_hadoop20.q 4961b97 
>   ql/src/test/results/clientpositive/ctas_hadoop20.q.out 885e985 
>   ql/src/test/results/clientpositive/literal_decimal.q.out 2d1ae55 
> 
> Diff: https://reviews.apache.org/r/15217/diff/
> 
> 
> Testing
> -------
> 
> New tests are added. Test suite passed.
> 
> 
> Thanks,
> 
> Xuefu Zhang
> 
>

Re: Review Request 15217: HIVE-5726: The DecimalTypeInfo instance associated with a decimal constant is not in line with the precision/scale of the constant

Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15217/#review28202
-----------------------------------------------------------


This looks very good!  Why didn't we add a new test to TestHiveDecimal? It looks like this functionality could be tested there.

- Brock Noland


On Nov. 4, 2013, 11:40 p.m., Xuefu Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15217/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2013, 11:40 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-5726
>     https://issues.apache.org/jira/browse/HIVE-5726
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> With this patch, the resuting schema from constant is in accordance with the constant instead of the default from the type.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/common/type/HiveDecimal.java 606f6d4 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java 47d7995 
>   ql/src/test/queries/clientpositive/ctas_hadoop20.q 4961b97 
>   ql/src/test/results/clientpositive/ctas_hadoop20.q.out 885e985 
>   ql/src/test/results/clientpositive/literal_decimal.q.out 2d1ae55 
> 
> Diff: https://reviews.apache.org/r/15217/diff/
> 
> 
> Testing
> -------
> 
> New tests are added. Test suite passed.
> 
> 
> Thanks,
> 
> Xuefu Zhang
> 
>