Michal Ostrowski has posted comments on this change. ( http://gerrit.cloudera.org:8080/11535 )
Change subject: IMPALA-6661 Make NaN values equal for grouping purposes. ...................................................................... Patch Set 16: (5 comments) http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/codegen/codegen-anyval.cc@469 PS16, Line 469: void CodegenAnyVal::ConvertToCanonicalForm() { > This code handles the codegen case. Is there an interpreted code path chang Yes. See changes in hash-table.cc http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/codegen/codegen-anyval.cc@728 PS16, Line 728: llvm::Value* CodegenAnyVal::EqToNativePtr(llvm::Value* native_ptr, > Maybe add a comment to define inclusive equality? The description in the co It's defined in the header comments http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/codegen/codegen-anyval.cc@751 PS16, Line 751: // Mirror logic in HashTableCtx::Equals - IMPALA-6661 > Maybe explain why we're mirroring the logic? So that two NaN compare as equ This is the codegen path. HashTableCtx::Equals is the corresponding non-codegen path. http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/exec/hash-table.cc@238 PS16, Line 238: bool HashTableCtx::Equals(const TupleRow* build_row, const uint8_t* expr_values, > Per earlier comment, maybe a comment here about what "INCLUSIVE" means? (Pa Defined in the header comments. http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3074 PS16, Line 3074: ---- QUERY > A quick check of this file found no cases where we test base NaN functional I've assumed that NaN tests have existed in other places. Exhaustive validation of testing of NaN is out-of-scope of this change. -- To view, visit http://gerrit.cloudera.org:8080/11535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 Gerrit-Change-Number: 11535 Gerrit-PatchSet: 16 Gerrit-Owner: Michal Ostrowski <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Michal Ostrowski <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 19 Oct 2018 01:50:05 +0000 Gerrit-HasComments: Yes
