anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 )
Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ...................................................................... Patch Set 11: (16 comments) http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/expr-test.cc@585 PS11, Line 585: ASSERT_TRUE(status.msg().msg().compare(status.msg().msg().size() - status.msg().msg() is prepended by SQLSTATE_GENERAL_ERROR string. I am using to compare to check if status.msg().msg() ends with the error string. https://github.com/apache/impala/blob/19ab465b391167e99658c9270a2a3d76b2b2652f/be/src/service/impala-server.cc#L231 http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@438 PS10, Line 438: num_buc > nit: should be num_buckets instead of buckets Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@444 PS10, Line 444: result > nit: "results" Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@455 PS10, Line 455: a > nit: "an overflow" Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@482 PS10, Line 482: if (UNLIKELY(min_range >= max_range)) { > UNLIKELY Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@487 PS10, Line 487: if (UNLIKELY(expr < min_range)) return 0; > UNLIKELY Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@489 PS10, Line 489: if (UNLIKELY(expr >= max_range)) { > UNLIKELY Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@504 PS10, Line 504: input_scale, input_precision, input_scale, false, &overflow); > UNLIKELY(overflow) Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@505 PS10, Line 505: f > I think this check needs to happen above the line 487. Imagine if this over If expr < min_range, we return 0 so If I move this above l487 it will start returning null if max_range - min_range overflows. If expr < min_range why should we perform this subtraction if the bucket number can be evaluated without performing this subtraction http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@507 PS10, Line 507: error_msg << "Overflow while evaluating the difference between min_range: " << > Add an end to end test for this error message (and others). Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@518 PS10, Line 518: Decimal16Value buckets = Decimal16Value::FromInt(input_precision, input_scale, > Add an end to end test for this error message. Since we won't hit this without overflowing above. Added a test where expr - min will overflow, but we detect the overflow during subtraction above and return. http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@536 PS10, Line 536: > I am not convinced this check is correct. In which case will this check be Removed and added the leading zero check. http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@536 PS10, Line 536: > I am not convinced this check is correct. In which case will this check be Removed this and just added the leading zero check. http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@538 PS10, Line 538: if (needs_int256) { > Is it possible to tell statically that needs_int256 is false? For example, Added the leading zero check to detect if we need int256. Would it suffice? http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@566 PS10, Line 566: const DecimalVal& min_range, const DecimalVal& max_range, > We need to DCHECK that it did not overflow here. (It should be easy to do i Done http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions.h File be/src/exprs/math-functions.h: http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions.h@115 PS10, Line 115: static BigIntVal WidthBucket(FunctionContext* ctx, const DecimalVal& expr, > Does this need to be declared in this file? Isn't it an internal function? I made this private. Not sure, any pointers where I can move this function to? -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 11 Gerrit-Owner: anujphadke <apha...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Thu, 22 Feb 2018 00:26:32 +0000 Gerrit-HasComments: Yes
