You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Kevin Wilfong <ke...@fb.com> on 2011/08/16 01:37:16 UTC
Review Request: Warn user that precision is lost when bigint is implicitly
cast to double.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1515/
-----------------------------------------------------------
Review request for hive and Siying Dong.
Summary
-------
I added a check in the code for equality expressions (includes inequalities) with operands of different types, that throws an error or logs a warning, depending on strict mode, if one operand is a string or double and the other is a bigint.
This addresses bug HIVE-2378.
https://issues.apache.org/jira/browse/HIVE-2378
Diffs
-----
trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ErrorMsg.java 1157946
trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseCompare.java 1157946
trunk/ql/src/test/queries/clientnegative/compare_double_bigint.q PRE-CREATION
trunk/ql/src/test/queries/clientnegative/compare_string_bigint.q PRE-CREATION
trunk/ql/src/test/results/clientnegative/compare_double_bigint.q.out PRE-CREATION
trunk/ql/src/test/results/clientnegative/compare_string_bigint.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/1515/diff
Testing
-------
I added two tests (one for strings and one for doubles) to record the issue.
I also verified the unit tests still run.
Thanks,
Kevin
Re: Review Request: Warn user that precision is lost when bigint is
implicitly cast to double.
Posted by Siying Dong <si...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1515/#review1510
-----------------------------------------------------------
It's OK if you think it's equivalent. Just make sure you print to console's error output stream, instead of Log.WARN
- Siying
On 2011-08-17 18:34:44, Kevin Wilfong wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1515/
> -----------------------------------------------------------
>
> (Updated 2011-08-17 18:34:44)
>
>
> Review request for hive and Siying Dong.
>
>
> Summary
> -------
>
> I added a check in the code for equality expressions (includes inequalities) with operands of different types, that throws an error or logs a warning, depending on strict mode, if one operand is a string or double and the other is a bigint.
>
>
> This addresses bug HIVE-2378.
> https://issues.apache.org/jira/browse/HIVE-2378
>
>
> Diffs
> -----
>
> trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ErrorMsg.java 1158835
> trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseCompare.java 1158835
> trunk/ql/src/test/queries/clientnegative/compare_double_bigint.q PRE-CREATION
> trunk/ql/src/test/queries/clientnegative/compare_string_bigint.q PRE-CREATION
> trunk/ql/src/test/results/clientnegative/compare_double_bigint.q.out PRE-CREATION
> trunk/ql/src/test/results/clientnegative/compare_string_bigint.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/1515/diff
>
>
> Testing
> -------
>
> I added two tests (one for strings and one for doubles) to record the issue.
>
> I also verified the unit tests still run.
>
>
> Thanks,
>
> Kevin
>
>
Re: Review Request: Warn user that precision is lost when bigint is
implicitly cast to double.
Posted by Kevin Wilfong <ke...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1515/
-----------------------------------------------------------
(Updated 2011-08-18 17:21:26.213141)
Review request for hive and Siying Dong.
Changes
-------
Changed my mind.
Ran a few more tests to make sure they were equivalent and found that having the check in GenericUDFBaseCompare.ObjectInspector.initialize can result in the warning being thrown multiple times for the same operator. I also found that queries, like the ones in the .q file don't hit DefaultExprProcessor.getFuncExprNodeDesc.
I added the check to ExprNodeGenericFuncDesc.newInstance because this is hit, it appears to only be hit once per operator, and it will be hit if the code ever goes through DefaultExprProcessor.getFuncExprNodeDesc.
Summary
-------
I added a check in the code for equality expressions (includes inequalities) with operands of different types, that throws an error or logs a warning, depending on strict mode, if one operand is a string or double and the other is a bigint.
This addresses bug HIVE-2378.
https://issues.apache.org/jira/browse/HIVE-2378
Diffs (updated)
-----
trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ErrorMsg.java 1158835
trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java 1158835
trunk/ql/src/test/queries/clientnegative/compare_double_bigint.q PRE-CREATION
trunk/ql/src/test/queries/clientnegative/compare_string_bigint.q PRE-CREATION
trunk/ql/src/test/results/clientnegative/compare_double_bigint.q.out PRE-CREATION
trunk/ql/src/test/results/clientnegative/compare_string_bigint.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/1515/diff
Testing
-------
I added two tests (one for strings and one for doubles) to record the issue.
I also verified the unit tests still run.
Thanks,
Kevin
Re: Review Request: Warn user that precision is lost when bigint is
implicitly cast to double.
Posted by Kevin Wilfong <ke...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1515/
-----------------------------------------------------------
(Updated 2011-08-17 18:34:44.253584)
Review request for hive and Siying Dong.
Changes
-------
This diff is in response to Siying's comments.
I'm a little hesitant to move the check into the method getFuncExprNodeDesc for two reasons:
1) if the name of the UDF the method is called on corresponds to a comparison operator the code I modified (and hence the check in GenericUDFBaseCompare.ObjectInspector.initialize()) is already called
DefaultExprProcessor.getFuncExprNodeDesc -> ExprNodeGenericFuncDesc.newInstance -> GenericUDFBaseCompare.ObjectInspector.initialize
2) I would have to write new code in getFuncExprNodeDesc to check if the GenericUDF is of type GenericUDFBaseCompare, and if so, get the TypeInfo for from each of its arguments. Admittedly, this is only a few lines, but we get this for free by putting the check in GenericUDFBaseCompare.ObjectInspector.initialize
If you still feel strongly about this, or you have other concerns, let me know and I will move the check, I just wanted to make those points before doing it.
I didn't realize we wanted to print the warning to the console. There is a function LogHelper.printError which checks uses SessionState's error output stream if possible and System.err otherwise and logs the message, which I have decided to use.
Summary
-------
I added a check in the code for equality expressions (includes inequalities) with operands of different types, that throws an error or logs a warning, depending on strict mode, if one operand is a string or double and the other is a bigint.
This addresses bug HIVE-2378.
https://issues.apache.org/jira/browse/HIVE-2378
Diffs (updated)
-----
trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ErrorMsg.java 1158835
trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseCompare.java 1158835
trunk/ql/src/test/queries/clientnegative/compare_double_bigint.q PRE-CREATION
trunk/ql/src/test/queries/clientnegative/compare_string_bigint.q PRE-CREATION
trunk/ql/src/test/results/clientnegative/compare_double_bigint.q.out PRE-CREATION
trunk/ql/src/test/results/clientnegative/compare_string_bigint.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/1515/diff
Testing
-------
I added two tests (one for strings and one for doubles) to record the issue.
I also verified the unit tests still run.
Thanks,
Kevin
Re: Review Request: Warn user that precision is lost when bigint is
implicitly cast to double.
Posted by Siying Dong <si...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1515/#review1503
-----------------------------------------------------------
Log.WARN is not sufficient to print error message to console. There is another place you can add the check in compile time: ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java in DefaultExprProcessor.getFuncExprNodeDesc(). There you can safely get error output stream from SessionState.get().
- Siying
On 2011-08-15 23:37:16, Kevin Wilfong wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1515/
> -----------------------------------------------------------
>
> (Updated 2011-08-15 23:37:16)
>
>
> Review request for hive and Siying Dong.
>
>
> Summary
> -------
>
> I added a check in the code for equality expressions (includes inequalities) with operands of different types, that throws an error or logs a warning, depending on strict mode, if one operand is a string or double and the other is a bigint.
>
>
> This addresses bug HIVE-2378.
> https://issues.apache.org/jira/browse/HIVE-2378
>
>
> Diffs
> -----
>
> trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ErrorMsg.java 1157946
> trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseCompare.java 1157946
> trunk/ql/src/test/queries/clientnegative/compare_double_bigint.q PRE-CREATION
> trunk/ql/src/test/queries/clientnegative/compare_string_bigint.q PRE-CREATION
> trunk/ql/src/test/results/clientnegative/compare_double_bigint.q.out PRE-CREATION
> trunk/ql/src/test/results/clientnegative/compare_string_bigint.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/1515/diff
>
>
> Testing
> -------
>
> I added two tests (one for strings and one for doubles) to record the issue.
>
> I also verified the unit tests still run.
>
>
> Thanks,
>
> Kevin
>
>