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 1: (25 comments) I need to tweak the decimal class a bit, but I updated based on the reviews. 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: internal types are represented and stored as > Could we if/def them out? I wasn't sure exactly how to do that in a way that wouldn't break the client. I really want to add this back but should do so after the initial patch. 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@64 PS6, Line 64: class KuduColumnTypeAttributes { > Nit: indentation Done http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@70 PS6, Line 70: > The 'explicit' keyword is only needed to avoid implicit conversions with si Done http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@75 PS6, Line 75: /// @return Precision for the column type. > These accessors are returning an int32_t 'copy', so what value does the 'co Done http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@84 PS6, Line 84: > Do you anticipate KuduColumnTypeAttributes evolving for other use cases in The only other attribute I can see being added as of now is "size". Though precision could be "reused" for that if we want. That would cover CHAR(size), VARCHAR(size), TIMESTAMP(precision) types if we want to add them in the future. http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@320 PS6, Line 320: /// > Add a note about the valid value ranges here and below. Done http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@71 PS1, Line 71: explicit > I don't think this is needed unless we want to protect against list-initial Done http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@76 PS1, Line 76: const > drop Done http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@81 PS1, Line 81: const > drop Done http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@321 PS1, Line 321: /// Clients must specify a precision for decimal columns. > Add the documented description for the precision parameter. Done http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@328 PS1, Line 328: /// Clients must specify a scale for decimal columns. > Add the documented description for the scale parameter. Done 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, other Done 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 concerne slice_val_ can't be added to the union because it has a non-trivial default constructor. I tried re-arranging the fields but regardless the sizeof data is 48. http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto@54 PS5, Line 54: DECIMAL32 = 17; > nit: could you add a comment to explain why 15 and 16 are skipped? Done 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: xported head > bikesheddy, but I'd mildly prefer plain 'Decimal' for this class name. If Done http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@32 PS6, Line 32: #include "kudu/client/stubs.h" > indentation Done http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@38 PS6, Line 38: > 'can be' here and below Done http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@57 PS6, Line 57: > I'd err on leaving this out of the public API unless there's a really compe Done http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@66 PS6, Line 66: // Returns a string representation of the DecimalValue with a given precision > I somewhat expected there to be basic math functions (at least add/subtract yeah, I think this should be a follow up. Decimal math functions can be tricky and the main motivation for decimal support is for integrations as opposed to direct use. Those integrations already have their own math implementations. 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: case DECIMAL32: > We should consider doing bounds checks here, at least in debug mode. Yeah, this is where I am not 100% sure what I/we should do. I could leave DecimalValue without precision/scale as it is now and validate the value "fits" here. Or I could have a precision/scale on DecimalValue and validate it matches the columns precision/scale here. Then all bounds checks would be a DecimalValue creation time. I just can't decide how "thin" DecimalValue should be. http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@50 PS1, Line 50: DataType type) const { > nit: misaligned line for the second parameter Done http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@57 PS1, Line 57: true > Why true, not false? If there is some specific reason, please add a commen Done http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@125 PS1, Line 125: return strings::Substitute("$0$1 $2", > nit: missed space This is on purpose.This is so the result is decimal(1, 2) instead of decimal (1, 2). 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 Done 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 I don't have a "special value" or field to indicate if the value is set on ColumnTypeAttributes. I could make that change, but I took the route of just defaulting to 0 so far. I don't feel too strongly either way. -- 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: 1 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 <danburk...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 09 Jan 2018 18:38:18 +0000 Gerrit-HasComments: Yes
