Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/8830 )
Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) ...................................................................... Patch Set 14: (5 comments) http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc File src/kudu/common/partial_row-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc@227 PS20, Line 227: // the copy constructor. > This error message may be clearer if it printed the scaled value instead of Done http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h@564 PS14, Line 564: return &MIN_UNSCALED_DECIMAL64; > Take a look at column_predicate-test for examples of type specific boundary I added a few tests in column_predicate-test but also simplified this code not to include the decimal "max" values but instead stick to the physical type maximums. In a follow up patch I will update places we use IsMinValue() and IsMaxValue() to take the ColumnTypeAtrributes into consideration and calculate the "real" limits for decimal columns. Until then the predicates are less than optimal but still correct. http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h File src/kudu/util/decimal_util.h: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h@63 PS20, Line 63: > Is having a default scale a benefit here? I'd expect that if the scale is True. I will remove the default. http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc File src/kudu/util/decimal_util.cc: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@28 PS20, Line 28: // 38 digits, 1 extra leading zero, decimal point, > Needs test Done http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@32 PS20, Line 32: int128_t n = d < 0? -d : d; > I think a for loop would be more idiomatic here. Done -- To view, visit http://gerrit.cloudera.org:8080/8830 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 14 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 31 Jan 2018 19:42:36 +0000 Gerrit-HasComments: Yes
