Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-2033: Added Netezza function: Width_Bucket. ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/1965/4//COMMIT_MSG Commit Message: Line 12: It would be nice if you had a short description of what width_bucket does here. http://gerrit.cloudera.org:8080/#/c/1965/4/be/src/exprs/decimal-functions.h File be/src/exprs/decimal-functions.h: Line 61: static void Width_Bucket_Prepare(FunctionContext*, FunctionContext::FunctionStateScope); You should either move this to math-functions.py or move the declaration of this version in impala_functions.py to be under the decimal functions header, for consistency. http://gerrit.cloudera.org:8080/#/c/1965/4/be/src/exprs/decimal-operators.h File be/src/exprs/decimal-operators.h: Line 26: class Expr; Why remove this line? Line 33: typedef FunctionContext::TypeDesc Decimal_Type; You should use this consistently here, eg. in the declaration of CastToTinyIntVal below. Line 45: static TinyIntVal CastToTinyIntVal(const DecimalVal&, const FunctionContext::TypeDesc&); Obviously its non-ideal to have two versions of so many functions in this file. Is there some way you could combine them? I don't even see where the existing versions of the functions are being used. http://gerrit.cloudera.org:8080/#/c/1965/4/be/src/exprs/math-functions.cc File be/src/exprs/math-functions.cc: Line 563: ctx->SetError("low boundary cannot equal to high boundary."); capitalize 'low' http://gerrit.cloudera.org:8080/#/c/1965/4/be/src/exprs/math-functions.h File be/src/exprs/math-functions.h: Line 121: extraneous newline? -- To view, visit http://gerrit.cloudera.org:8080/1965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If1ccd6999544010779b20e4008d37ab4c23151e6 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
