impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Date Mon, 26 Jun 2017 19:02:57 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6023/5/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

Line 452:   Decimal16Value buckets = Decimal16Value::FromInt(input_precision, input_scale,
It would be good to summarize the calculations and comment on the chosen types, e.g. why are
we using Dec16 here?

In general, please move definitions closer to where they are used. The buckets are only used
a lot further down where it might become clearer why it should be Dec16.

Adding those comments and doing some cleanup will make reviewing much easier. I'll still need
to think more carefully about the logic.


-- 
To view, visit http://gerrit.cloudera.org:8080/6023
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <aphadke@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message