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: (8 comments) http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc@231 PS14, Line 231: // Returns a vector of decimal(4, 2) numbers from -50.50 (inclusive) to 50.50 > This is still true. The values are scaled up by 100 because of the scale 2. d'oh! http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@256 PS14, Line 256: KuduColumnSchema(const std::string &name, > Todd had suggested I do this in a previous review. If we can't do this I ju OK fine by me then. 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. > Currently the scale must exactly match the columns scale when being used in Why not document that the scale has to match exactly then? 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) { > Yeah, perhaps. It depends on how strict we want to be with these KuduValues Thats fair. Probably worth a comment if you keep it as is. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc File src/kudu/common/partial_row-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@215 PS14, Line 215: EXPECT_EQ("decimal decimal_val=123456_D32", row.ToString()); > This is coming from TypeInfo.AppendDebugStringForValue which doesn't have t Yeah, this is going to be a very confusing output since it's unscaled and the _D32 suffix is unlike anything else. We do use this output in a few places, including the scans dashboard I think. 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: int128_t min_val, max_val; > Yeah, thats a better check here. Will update. Probably indicates a testing gap. 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; > Here we don't have the precision/scale information to work with since it ri Yeah I understand that, but the min_value/max_value contract is that it should return the min and max value, not some other value. There's a bunch of predicate code written with this assumption in mind, and I'm nervous that this is going to break it. I think it's worth adding more complete coverage of edge cases to column_predicate-test for the different bucket sizes to be sure. Also keep in mind we do special predicate optimizations just for PK columns. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.cc File src/kudu/util/decimal_util.cc: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.cc@27 PS14, Line 27: string DecimalToString(int128_t d, int8_t scale) { > You mean move this into gutil/strings/numbers.cc? disregard -- 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: Mon, 29 Jan 2018 22:50:26 +0000 Gerrit-HasComments: Yes
