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 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/8830/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8830/6//COMMIT_MSG@24 PS6, Line 24: This also removes the int128 suffixes because they break the Could we if/def them out? http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@320 PS6, Line 320: /// Clients must specify a precision for decimal columns. Add a note about the valid value ranges here and below. http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.cc@261 PS6, Line 261: if (data_->type == KuduColumnSchema::DECIMAL && !data_->has_scale) { I think this should be checking the scale/precision are within range, otherwise it could result in a CHECK failure later, right? http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/value-internal.h File src/kudu/client/value-internal.h: http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/value-internal.h@48 PS6, Line 48: Slice slice_val_; just curious - do you know why this isn't in the union? I'm mildly concerned that inflating the size of KuduValue::Data will have a perf impact, but it would be completely offset if we could put the slice into the union as well (since it's already 128 bits). edit: now that I'm looking at it, we could probably save a bunch of space just reordering to be slice_val_ union type_ in order to reduce padding waste. http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h File src/kudu/common/decimal_value.h: http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@31 PS6, Line 31: DecimalValue bikesheddy, but I'd mildly prefer plain 'Decimal' for this class name. If there's some prior-art that's already DecimalValue then that's fine too. http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@32 PS6, Line 32: public: indentation http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@38 PS6, Line 38: can will be 'can be' here and below http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@57 PS6, Line 57: static std::string Stringify(int precision, int scale, int128_t d); I'd err on leaving this out of the public API unless there's a really compelling reason to have it available. Just in the interest of keeping public client APIs thin, and it doesn't seem like going through ToString would have a perf impact (no allocation or anything). http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@63 PS6, Line 63: explicit DecimalValue(int128_t s) : Might be useful to have a simple example of how to use this class in the class doc, something along these lines: DecimalValue d(12345); CHECK_EQ("12.345", d.ToString(5, 2)); http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@66 PS6, Line 66: DecimalValue() : value_(static_cast<int128_t>(0)) { } I somewhat expected there to be basic math functions (at least add/subtract/multiply) defined. Maybe as a follow-up? http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/partial_row.cc@304 PS6, Line 304: return Set<TypeTraits<DECIMAL32> >(col_idx, static_cast<int32_t>(val.value())); We should consider doing bounds checks here, at least in debug mode. http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/schema.cc@50 PS6, Line 50: DataType type) const { indent http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/wire_protocol.cc@214 PS6, Line 214: pb->mutable_type_attributes()->set_precision(col_schema.type_attributes().precision); Is it possible to if guard these as well? That way the scale/precision won't get set, and won't get pretty printed in ToDebugString calls, etc. -- 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: 6 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 21 Dec 2017 01:14:27 +0000 Gerrit-HasComments: Yes
