Todd Lipcon 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 8: (23 comments) I think this could do with a couple end-to-end tests from the client writing decimals of different precision and making sure they come back reasonably http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@17 PS8, Line 17: stored not stored? it must be stored in the metadata somewhere right? http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@24 PS8, Line 24: int128 suffixes perhaps we should guard them with a preprocessor check for c++11 instead? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@63 PS8, Line 63: class KuduColumnTypeAttributes { should this be exported for users? If so I think we need to PIMPL it for ABI compatibility (I imagine this is where we might later add stuff like charsets for strings?) If not exported then I think it belongs in schema-internal.h or somesuch http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@177 PS8, Line 177: /// @todo KUDU-809: make this hard-to-use constructor private. : /// Clients should use the Builder API. Currently only the Python API : /// uses this old API. this has been deprecated for several releases now and I dont think python uses it anymore. Maybe we can remove it (make it private)? Changing its signature is equally as illegal as removing it. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@323 PS8, Line 323: floating-point it's not floating point, right? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@328 PS8, Line 328: /// The precision must be between 0 and 38. curious what a precision of 0 means... that can only store the value 0? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@343 PS8, Line 343: /// columns precision. typo: column's http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@133 PS8, Line 133: return kudu::DECIMAL128; nit: weird indentation http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@153 PS8, Line 153: case kudu::DECIMAL32 : return KuduColumnSchema::DECIMAL; nit: extra space http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@266 PS8, Line 266: return Status::InvalidArgument("no scale provided for decimal column", data_->name); is a default scale of 0 not reasonable? I seem to recall that sql allows DECIMAL(5) and that means 0 through 99999 http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@285 PS8, Line 285: } can we check that no precision/scale are specified for non-decimal column types? want to make sure someone doesn't try to do STRING(10) under some assumption this would produce a fixed-length or truncated string type (as in sql VARCHAR(10)) http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@287 PS8, Line 287: int32_t precision = 0; : if (data_->has_precision) { : precision = data_->precision; : } think ternary would be easier to read here and below http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@297 PS8, Line 297: KuduColumnTypeAttributes kuduColumnTypeAttributes = nit: var name style Also you can just construct this like KuduColumnTypeAttributes attr(precision, scale) -- no need for the extra assignment http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@641 PS8, Line 641: typeAttrs nit: style http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h File src/kudu/common/decimal.h: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@49 PS8, Line 49: 999999999999999999) * 100) + 99; // 38 9's well this is fun. have you tested that this works as expected and not getting secretly truncated somewhere along the line? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@53 PS8, Line 53: static const int32_t MIN_DECIMAL_PRECISION = 0; > warning: invalid case style for global constant 'MIN_DECIMAL_PRECISION' [re see comment elsewhere about precision 0 http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@63 PS8, Line 63: explicit Decimal(int128_t s) : I sort of feel like this Decimal instance should retain its precision and scale as a member and then check for compatibility in the appropriate spots. Otherwise it will be very easy to get incorrect results if you accidentally pass a non-matching decimal Value into something like a predicate, no? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc File src/kudu/common/decimal.cc: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@25 PS8, Line 25: int last_char_idx = precision nit: indentation off in this file http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@29 PS8, Line 29: string no need to repeat type name http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@30 PS8, Line 30: // Start filling in the values in reverse order by taking the last digit : // of the value. Use a positive value and worry about the sign later. At this : // point the last_char_idx points to the string terminator. : did this algorithm get ported from somewhere? is there a tried and true one in sqlite or somesuch? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/types.h@558 PS8, Line 558: // to format it. maybe we should still format it in such a way that it's clear the value being written is not actually a normal int? eg 12345/? or somesuch? or D12345? 12345D? '12345e?' ? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc@214 PS8, Line 214: pb->mutable_type_attributes()->set_precision(col_schema.type_attributes().precision); maybe we should only serialize them in the case that it is decimal, to save space/deserialization cost? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc@264 PS8, Line 264: typeAttributes style (also below) -- 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: 8 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: Thu, 11 Jan 2018 03:11:49 +0000 Gerrit-HasComments: Yes
