Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 )
Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/expr-test.cc@4247 PS7, Line 4247: TestValue("width_bucket(6.3, 2, 17, 2)", TYPE_BIGINT, 1); You should include a case like (10000, 1, 6, 3) http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc@429 PS7, Line 429: bucket_width This should be called bucket_number to make it more clear http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc@431 PS7, Line 431: width_size width_size is a confusing name. This should be called something like "distance_from_min". http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc@516 PS7, Line 516: int256_t x = ConvertToInt256(buckets.value()) * ConvertToInt256(width_size.value()); > Won't this require a lot of memory to create such a array? Sure, that's true. I don't think that this function is meant to be used that way though. It's difficult to imagine someone wanting to use over 1000 buckets. In that case, we could fall back to the current implementation. -- 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: 7 Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Tue, 10 Oct 2017 00:41:56 +0000 Gerrit-HasComments: Yes
