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 3: (19 comments) http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.h File be/src/codegen/codegen-anyval.h: http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.h@236 PS3, Line 236: ensusre > enusre Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.h@240 PS3, Line 240: > Extra blank line. Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.cc@473 PS3, Line 473: static llvm::APFloat canonical_float_nan(RawValue::CANONICAL_FLOAT_NAN); > This is a constant, right? Mark it const and make the name upper case. Done - actually redundant - cruft from an earlier rev. http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.cc@476 PS3, Line 476: case TYPE_FLOAT: > nit: we usually indent the cases (see elsewhere in this file). Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.cc@731 PS3, Line 731: bool inclusive_equality) { > Indentation of continuation is a bit inconsistent. We usually either have 4 Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.cc@751 PS3, Line 751: if (!inclusive_equality) { > The house style is to put conditionals in a single line if they fit, i.e. Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.cc@756 PS3, Line 756: local_val, "local_val_is_nan"); > Inconsistent indentation of continuation line. Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/exec/hash-table.h File be/src/exec/hash-table.h: http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/exec/hash-table.h@463 PS3, Line 463: /// values in 'expr_values' with nullness 'expr_values_null'. INCLUSIVE_EQUALITY broadens > Not a big deal, but you can prefetch these comments before uploading a patc Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/exec/hash-table.cc@198 PS3, Line 198: auto > optional: We often prefer spelling out the type name if it's not too verbos Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/exec/hash-table.cc@255 PS3, Line 255: (!INCLUSIVE_EQUALITY || > Indentation of the conditional here is a little inconsistent with other Imp This was intentional for readability, since compression here really does impede readability IMO. http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.h File be/src/runtime/raw-value.h: http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.h@110 PS3, Line 110: /// that is considered representative of the same ambiguous value. > This should only be called is IsAmbiguous() returns true for the type, righ IsAmbiguous() returns true if a *particular value of a particular type* is ambiguous. "CanonicalValue" is the single canonical value for a given type. http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.inline.h File be/src/runtime/raw-value.inline.h: http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.inline.h@52 PS3, Line 52: return false; > Not reachable Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.inline.h@62 PS3, Line 62: return NULL; > Should this have a DCHECK(false)? Since it shouldn't be called for other ty Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.inline.h@64 PS3, Line 64: return NULL; > Not reachable Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.inline.h@172 PS3, Line 172: if (std::isnan(*v)) { > Could fit on one line. Done http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.inline.h@183 PS3, Line 183: if (std::isnan(*v)) { > Could fit on one line. Done http://gerrit.cloudera.org:8080/#/c/11535/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/11535/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test@12 PS3, Line 12: # GROUP BY of NaN values aggregates NaN's as one grouping > Can you also add an end-to-end test for float? Done http://gerrit.cloudera.org:8080/#/c/11535/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test@13 PS3, Line 13: ; > Trailing semicolon not needed in .test Done http://gerrit.cloudera.org:8080/#/c/11535/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test@13 PS3, Line 13: select count(*), sqrt(0.5-x) as Z from (VALUES((1.6 x, 2 y), (3.2, 4), (5.4,6))) T group by Z; > Can you add some line breaks to put the from and group by clauses on differ Done -- 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: 3 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: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 01 Oct 2018 21:12:41 +0000 Gerrit-HasComments: Yes
