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 17: (4 comments) http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h File src/kudu/client/value.h: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h@63 PS14, Line 63: /// The decimal value's scale. > Why not document that the scale has to match exactly then? Done 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: > Thats fair. Probably worth a comment if you keep it as is. I think I will make this strict too so everything starts strict. We can then loosen up the rules with well thought out coercion. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row.cc@305 PS14, Line 305: > Probably indicates a testing gap. I added a quick test in the update but will add some more bounds tests. 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 &kMinUnscaledDecimal64; > Yeah I understand that, but the min_value/max_value contract is that it sho The way I looked at this is that this was the min_value/max_value that could be stored in the type regardless of the type attributes. Any type attributes only restrict the range further. I looked at the predicate code and we use the min_value/max_value to optimize the predicates by converting range predicates into IsNotNull or Equality predicates. Since the precision would only reduce the value set this shouldn't be an issue. I have a predicate test that check boundary values and values between of a decimal value. What types of additional tests should I add? -- 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: 17 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 03:44:54 +0000 Gerrit-HasComments: Yes
