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 10: (24 comments) http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h File be/src/codegen/codegen-anyval.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h@236 PS9, Line 236: ensure > ensures Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h@239 PS9, Line 239: /// value of the value. (Currently this only has an impact for NaN > May want to clarify that this is a no-op for types other than FLOAT and DOU Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h@240 PS9, Line 240: loat and double values > ConvertNanToCanonicalForm(); Intentionally not named with "NaN" because the interface as defined now does not depend on it. It just so happens that for non-NaN, non-double/float values it is a no-op. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h@249 PS9, Line 249: clusive_equality" > Warrants a quick comment. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@476 PS9, Line 476: llvm::Value* > llvm::Value* . Same below. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@477 PS9, Line 477: > Seems unnecessary to initialize this value here as it's set in all paths wh Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@483 PS9, Line 483: is_n > "is_nan" is a more fitting name. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@487 PS9, Line 487: default: : ; > May as well remove these lines. Needed to avoid compiler errors/warnings. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@747 PS9, Line 747: llvm::Value* > llvm::Value* . Same below. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@748 PS9, Line 748: llvm::Value* cmp_raw = builder_->CreateFCmpOEQ(local_val, val, "cmp_raw"); : if (!inclusive_equality) return cmp_raw; > nit: seems slightly easier to follow: I need to generate that comparison in either case, so if I followed you suggestion I'd need to follow that with: llvm::Value *cmp_raw = builder_->CreateFCmpOEQ(local_val, val, "cmp_raw"); Note that the current version uses "cmp_raw" in 2 places. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.h File be/src/exec/hash-table.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.h@464 PS9, Line 464: means "NULL==NULL" and "NaN==NaN". This > means "NULL == NULL" and "Nan == Nan" Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@198 PS9, Line 198: const ColumnType& > nit: const ColumnType& expr_type Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@257 PS9, Line 257: = R > huh ? Did you mean val ? Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@801 PS9, Line 801: > Don't we need to check for inclusive_equality flag here ? Also, why is the There is no "inclusive_equality" flag here. The restriction to probe-only is a continuation of the original logic for NULL's, which was based on that distinction. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@1149 PS9, Line 1149: } : if (inclusive_equality) result.ConvertToCanonicalForm(); : > nit: one line Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@1179 PS9, Line 1179: } > nit: extra blank line Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h File be/src/runtime/raw-value.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@40 PS9, Line 40: /// Single NaN values to ensure all NaN values can be assigned one bit pattern : /// that will always compare and hash t > These deserve some comments on their purposes. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@107 PS9, Line 107: there are > IsNan() seems to be a more appropriate name. The name and the description o The ambiguity is intentional so as to not define float/double-specific mechanisms into a generic interface. By keeping the ambiguity it allows for the interfaces to be used unconditionally, irrespective of the type of value being represented by a RawValue object. If these mechanisms were specifically defined for float/double, then the caller would have to consider whether or not it should use the mechanisms. By keeping it ambiguous the interfaces can be used unconditionally, and simply result in no-ops for non-float/non-double values. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@112 PS9, Line 112: represents a > CanonicalNanValue(). Ack http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@112 PS9, Line 112: ingle bit pattern > nit: const ColumnType& type Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.inline.h File be/src/runtime/raw-value.inline.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.inline.h@34 PS9, Line 34: namespace impala { > nit: blank line. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.inline.h@50 PS9, Line 50: > Calling this function with a type other than float or double seems legitima Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.inline.h@63 PS9, Line 63: > nullptr Done http://gerrit.cloudera.org:8080/#/c/11535/9/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/11535/9/testdata/workloads/functional-query/queries/QueryTest/exprs.test@25 PS9, Line 25: group by Z > May warrant some test cases which group by multiple columns, Some columns w 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: 10 Gerrit-Owner: Michal Ostrowski <mostr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Michal Ostrowski <mostr...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 10 Oct 2018 15:05:13 +0000 Gerrit-HasComments: Yes
