Mailing-List: contact commits-help@impala.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: abehm@apache.org
To: commits@impala.apache.org
Date: Fri, 08 Dec 2017 23:28:00 -0000
Message-Id: <58ae376cb7ba480f9903876d3be16716@git.apache.org>
Subject: [1/2] impala git commit: IMPALA-6069: Fix CodegenAnyVal's handling of
 'nan'
archived-at: Fri, 08 Dec 2017 23:28:03 -0000

Repository: impala
Updated Branches:
  refs/heads/master 542dc227c -> 11497c2aa


IMPALA-6069: Fix CodegenAnyVal's handling of 'nan'

Previously, CodegenAnyVal used an LLVM function for floating point
comparison that considered 'nan' = 'nan' to be true. This is
inconsistent with the way we handle 'nan' in the non-codegen path,
where we consider 'nan' = 'nan' to be false, leading to inconsisent
results.

This patch fixes CodegenAnyVal to use an LLVM function for floating
point comparison that considers 'nan' = 'nan' to be false.

Testing:
- Added e2e tests for the two scenarios affected by this: CASE and
  joins.

Change-Id: I1bb8e5074b3c939927dedc46bc9db63ca24486a1
Reviewed-on: http://gerrit.cloudera.org:8080/8790
Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com>
Reviewed-by: Michael Ho <kwho@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/e94c6083
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/e94c6083
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/e94c6083

Branch: refs/heads/master
Commit: e94c60833a36728ae9ecbf22c161c0d7f41a7184
Parents: 542dc22
Author: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Authored: Thu Dec 7 11:20:46 2017 -0800
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
Committed: Fri Dec 8 22:42:03 2017 +0000

----------------------------------------------------------------------
 be/src/codegen/codegen-anyval.cc                |  6 +++--
 .../queries/QueryTest/exprs.test                |  8 +++++++
 .../queries/QueryTest/joins.test                | 24 ++++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/e94c6083/be/src/codegen/codegen-anyval.cc
----------------------------------------------------------------------
diff --git a/be/src/codegen/codegen-anyval.cc b/be/src/codegen/codegen-anyval.cc
index 45e8ee3..f7edec2 100644
--- a/be/src/codegen/codegen-anyval.cc
+++ b/be/src/codegen/codegen-anyval.cc
@@ -676,7 +676,8 @@ llvm::Value* CodegenAnyVal::Eq(CodegenAnyVal* other) {
       return builder_->CreateICmpEQ(GetVal(), other->GetVal(), "eq");
     case TYPE_FLOAT:
     case TYPE_DOUBLE:
-      return builder_->CreateFCmpUEQ(GetVal(), other->GetVal(), "eq");
+      // Use the ordering version "OEQ" to ensure that 'nan' != 'nan'.
+      return builder_->CreateFCmpOEQ(GetVal(), other->GetVal(), "eq");
     case TYPE_STRING:
     case TYPE_VARCHAR:
     case TYPE_FIXED_UDA_INTERMEDIATE: {
@@ -716,7 +717,8 @@ llvm::Value* CodegenAnyVal::EqToNativePtr(llvm::Value* native_ptr) {
       return builder_->CreateICmpEQ(GetVal(), val, "cmp_raw");
     case TYPE_FLOAT:
     case TYPE_DOUBLE:
-      return builder_->CreateFCmpUEQ(GetVal(), val, "cmp_raw");
+      // Use the ordering version "OEQ" to ensure that 'nan' != 'nan'.
+      return builder_->CreateFCmpOEQ(GetVal(), val, "cmp_raw");
     case TYPE_STRING:
     case TYPE_VARCHAR:
     case TYPE_FIXED_UDA_INTERMEDIATE: {

http://git-wip-us.apache.org/repos/asf/impala/blob/e94c6083/testdata/workloads/functional-query/queries/QueryTest/exprs.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/exprs.test b/testdata/workloads/functional-query/queries/QueryTest/exprs.test
index f0b9fac..0ed71ee 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/exprs.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/exprs.test
@@ -2902,3 +2902,11 @@ where a.id = 100
 ---- TYPES
 INT,INT,INT
 ====
+---- QUERY
+select id from functional.alltypes
+where id = case cast('nan' as double) when cast('nan' as double) then 0 else 1 end
+---- RESULTS
+1
+---- TYPES
+INT
+====
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/impala/blob/e94c6083/testdata/workloads/functional-query/queries/QueryTest/joins.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/joins.test b/testdata/workloads/functional-query/queries/QueryTest/joins.test
index ab2a531..643c3b1 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/joins.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/joins.test
@@ -771,3 +771,27 @@ select count(*) from (
 ---- TYPES
 BIGINT
 ====
+---- QUERY
+# Test that 'nan' != 'nan' when joining.
+with x as (select cast('nan' as double) n)
+select * from x a, x b where a.n = b.n
+---- RESULTS
+---- TYPES
+DOUBLE,DOUBLE
+====
+---- QUERY
+# Test that 'nan' != 'nan' when joining.
+select a.id from functional.alltypestiny a, functional.alltypestiny b
+where 1/a.double_col + -1/a.double_col = log10(-b.id)
+---- RESULTS
+---- TYPES
+INT
+====
+---- QUERY
+# Test that 'nan' != 'nan' when joining.
+select a.id from functional.alltypestiny a, functional.alltypestiny b
+where sqrt(-a.id) = b.float_col / b.double_col
+---- RESULTS
+---- TYPES
+INT
+====