impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6069: Fix CodegenAnyVal's handling of 'nan'
Date Thu, 07 Dec 2017 20:19:36 GMT
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 20:19:36 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message