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 3: (20 comments) Mostly comments about style and testing. Some of the style comments probably seem pedantic since your code is perfectly readable but we've generally favoured keeping the style around indentation, etc, as consistent as possible. Usually I'd look for the style to be consistent with the existing code. It's also ok if you ran it through clang-format and it did something slightly inconsistent so long as it's readable (having formatting automated by a tool avoids a lot of these issues ). 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 http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.h@240 PS3, Line 240: Extra blank line. 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. 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). http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/codegen/codegen-anyval.cc@490 PS3, Line 490: ; Semicolon needed? 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 space indents for continuations or go with whatever clang-format chooses. 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. if (!inclusive_equality) return cmp_raw; 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. 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 > line too long (91 > 90) Not a big deal, but you can prefetch these comments before uploading a patch: ./bin/jenkins/critique-gerrit-review.py --dryrun 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 verbose so it's more obvious what the type is. 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 Impala code, since we'd usually try to fit the clauses onto as few lines as possible. I don't feel strongly since this is perfectly readable but others might feel more strongly about consistency. 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, right? 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 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 types. http://gerrit.cloudera.org:8080/#/c/11535/3/be/src/runtime/raw-value.inline.h@64 PS3, Line 64: return NULL; Not reachable 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. 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. 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? 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 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 different lines. -- 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 20:38:09 +0000 Gerrit-HasComments: Yes
