Dan Burkert 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: (2 comments) http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc File src/kudu/client/value.cc: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@248 PS14, Line 248: if (decimal_val_ < min_val || decimal_val_ > max_val) { > I think I will make this strict too so everything starts strict. We can the SGTM 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; > The way I looked at this is that this was the min_value/max_value that coul Take a look at column_predicate-test for examples of type specific boundary checks that would be useful to have for decimal. I'd also like to see this extended for each class of decimal. We've been bitten a few times in the past by incomplete edge case coverage on predicates, which is why the tests are so extensive / verbose now. I think you're right that the current implementation should be fine, but I'd feel better with more coverage. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 30 Jan 2018 22:13:51 +0000 Gerrit-HasComments: Yes
