Tim Armstrong 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: Code-Review+1 (2 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@751 PS16, Line 751: // Mirror logic in HashTableCtx::Equals - IMPALA-6661 > This is the codegen path. HashTableCtx::Equals is the corresponding non-co Yeah this is kind of a cross-cutting feature of codegen. Long-term we'd like to minimise the duplication of logic, e.g. by cross-compiling C++ code into LLVM IR and substituting constants (you can go a long way with this if the the constants are complex things like descriptor tables that are variable in the interpreted code but constant in the codegen'd version). 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 > The issue in this bug is that we want "GROUP BY" to consider NaN==NaN, but expr-test.cc is probably the right place for exhaustive operator tests. Those get run with and without codegen so there's no trickier required to ensure that both code paths are tested. Seems worth documenting the test gap in a JIRA and fixing it soonish just to get the extra coverage - this patch is close enough to being done that I don't want to hold it up to tack on something tangential. -- 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: Tue, 23 Oct 2018 20:15:34 +0000 Gerrit-HasComments: Yes
