Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8790 )
Change subject: IMPALA-6069: Fix CodegenAnyVal's handling of 'nan' ...................................................................... Patch Set 1: (2 comments) Seems like the right fix. http://gerrit.cloudera.org:8080/#/c/8790/1/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/8790/1/be/src/codegen/codegen-anyval.cc@679 PS1, Line 679: return builder_->CreateFCmpOEQ(GetVal(), other->GetVal(), "eq"); Can you add a comment explaining why this variant is used? It's subtle. I looked at the LLVM language reference manual to make sure the semantics were correct: https://llvm.org/docs/LangRef.html#fcmp-instruction http://gerrit.cloudera.org:8080/#/c/8790/1/testdata/workloads/functional-query/queries/QueryTest/joins.test File testdata/workloads/functional-query/queries/QueryTest/joins.test: http://gerrit.cloudera.org:8080/#/c/8790/1/testdata/workloads/functional-query/queries/QueryTest/joins.test@775 PS1, Line 775: # Test that 'nan' != 'nan' when joining. Can we test a few more NaN variants just to get additional coverage? I think in practice the code path is the same now, but if our constant replacement got more sophisticated that might change. I'm able to get NaNs from expressions on alltypestiny in a few ways. I used this for ideas https://en.wikipedia.org/wiki/NaN#Operations_generating_NaN: select sqrt(-int_col), float_col / double_col, 0 * 1/double_col, 1/double_col + -1/double_col, log10(-double_col) from functional.alltypestiny -- To view, visit http://gerrit.cloudera.org:8080/8790 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1bb8e5074b3c939927dedc46bc9db63ca24486a1 Gerrit-Change-Number: 8790 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Dec 2017 20:19:36 +0000 Gerrit-HasComments: Yes
