[Impala-CR](cdh5-2.6.0_5.8.0) IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal().
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). .. IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). Numeric literals with a decimal point are typed as DECIMAL, if possible. Since decimals have a higher processing cost than FLOAT/DOUBLE we have special casting rules to convert DECIMAL operations to DOUBLE in certain reasonable circumstances. The bug was that in this conversion process we used to blindly replace all numeric literals with DOUBLE. The blind conversion caused expressions to fail analysis after the substitution if they did not have a matching DOUBLE signature. For example, round() requies the second argument to be an integer, and does not have a signature with DOUBLE as the second argument. The fix is to only replace DECIMAL literals with DOUBLE. Based on the comments in the relevant parts of the code, this new behavior matches the original intent more cleanly/directly. Testing: I did a private core/hdfs run and local testing. Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Reviewed-on: http://gerrit.cloudera.org:8080/3138 Reviewed-by: Alex Behm Tested-by: Internal Jenkins --- M fe/src/main/java/com/cloudera/impala/analysis/Expr.java M fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java 2 files changed, 10 insertions(+), 6 deletions(-) Approvals: Internal Jenkins: Verified Alex Behm: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/3138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-CR](cdh5-2.6.0_5.8.0) IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal().
Alex Behm has posted comments on this change. Change subject: IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-CR](cdh5-2.6.0_5.8.0) IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal().
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3138 to look at the new patch set (#2). Change subject: IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). .. IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). Numeric literals with a decimal point are typed as DECIMAL, if possible. Since decimals have a higher processing cost than FLOAT/DOUBLE we have special casting rules to convert DECIMAL operations to DOUBLE in certain reasonable circumstances. The bug was that in this conversion process we used to blindly replace all numeric literals with DOUBLE. The blind conversion caused expressions to fail analysis after the substitution if they did not have a matching DOUBLE signature. For example, round() requies the second argument to be an integer, and does not have a signature with DOUBLE as the second argument. The fix is to only replace DECIMAL literals with DOUBLE. Based on the comments in the relevant parts of the code, this new behavior matches the original intent more cleanly/directly. Testing: I did a private core/hdfs run and local testing. Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 --- M fe/src/main/java/com/cloudera/impala/analysis/Expr.java M fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java 2 files changed, 10 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/38/3138/2 -- To view, visit http://gerrit.cloudera.org:8080/3138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong
[Impala-CR](cdh5-2.6.0_5.8.0) IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal().
Tim Armstrong has posted comments on this change. Change subject: IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/3138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-CR](cdh5-2.6.0_5.8.0) IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal().
Alex Behm has posted comments on this change. Change subject: IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). .. Patch Set 1: I agree that convertNumericLiteralsFromDecimal() possibly doesn't make sense at all, but let's tackle that separately in IMPALA-3437. This patch has no effect on the results and types of the examples in IMPALA-3437. -- To view, visit http://gerrit.cloudera.org:8080/3138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-CR](cdh5-2.6.0_5.8.0) IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal().
Tim Armstrong has posted comments on this change. Change subject: IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). .. Patch Set 1: I'm not sure that convertNumericLiteralsFromDecimal() makes sense in general, but I'm also not sure we can fix it without breaking compatibility. See IMPALA-3437 Does this patch affect the results of either of the examples in IMPALA-3437? -- To view, visit http://gerrit.cloudera.org:8080/3138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-CR](cdh5-2.6.0_5.8.0) IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal().
Alex Behm has uploaded a new change for review. http://gerrit.cloudera.org:8080/3138 Change subject: IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). .. IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal(). Numeric literals with a decimal point are typed as DECIMAL, if possible. Since decimals have a higher processing cost than FLOAT/DOUBLE we have special casting rules to convert DECIMAL operations to DOUBLE in certain reasonable circumstances. The bug was that in this conversion process we used to blindly replace all numeric literals with DOUBLE. The blind conversion caused expressions to fail analysis after the substitution if they did not have a matching DOUBLE signature. For example, round() requies the second argument to be an integer, and does not have a signature with DOUBLE as the second argument. The fix is to only replace DECIMAL literals with DOUBLE. Based on the comments in the relevant parts of the code, this new behavior matches the original intent more cleanly/directly. Testing: I did a private core/hdfs run and local testing. Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 --- M fe/src/main/java/com/cloudera/impala/analysis/Expr.java M fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java 2 files changed, 10 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/38/3138/1 -- To view, visit http://gerrit.cloudera.org:8080/3138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Alex Behm
