Paul Rogers 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) Good stuff. A few random 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 change that is needed for when codegen is disabled? 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 commit message is good, maybe just add it here for future reference. 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 equal? Or, so that they have the same hash code? Both? 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? (Parallel handling of NULL and NaN.) 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 functionality. According to Wikipedia(https://en.wikipedia.org/wiki/NaN), NaN has specific rules. Here we test IS DISTINCT, but should we add base tests for other operators? Having such tests will catch any regressions in the future if someone accidentally turns on the new mode at the wrong time. -- 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: Thu, 18 Oct 2018 22:11:14 +0000 Gerrit-HasComments: Yes
