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: (18 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 Update this, since the for-loop is striding by 100. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc@241 PS14, Line 241: values.push_back(0); 0 should already be added by the for-loop above. 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, Unfortunately I don't think you can move/change this API in this way, even though it's deprecated. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@334 PS14, Line 334: represented by 'represented' is used twice in the sentence in slightly different ways, which may be confusing http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@343 PS14, Line 343: KuduColumnSpec* Precision(int8_t precision); Is there a default value? If so doc it. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@351 PS14, Line 351: 0 and 0.999... : /// or 0 and -0.999... it's not or, right? Just -0.9999 to 0.9999 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. Is there a restriction that scale must be < the column's scale? If so add that note. 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@221 PS14, Line 221: // TODO: Coerce when possible Just for my own understanding, is this coercion possible when scale < type_attributes.scale? http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@244 PS14, Line 244: Substitute("invalid type $0 provided for column '$1' (expected DECIMAL)", May want to put this check before the scale check, since I'd expect to get a type error instead of a scale error for a non-decimal column. 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) { Shouldn't this be checking against type_attributes.precision, not the min/max for that size bucket? 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@43 PS14, Line 43: ColumnSchema("decimal_val", DECIMAL32, true, NULL, NULL, s/NULL/nullptr/g 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 kind of an odd representation. The column schema / type attributes are available, right? Can it be something like 'decimal(x, y) decimal_val=123456'? http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc File src/kudu/common/partial_row-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc@20 PS15, Line 20: #include <string> use <cstddef> 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; Same here, should we check against the column precision? 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; This also goes back to the precision from the column vs min/max for the bucket thing. This min_value/max_value is used for optimizing predicates, and this is going to play into it. I think what we're doing here is technically correct, but not optimal. We can discuss on chat, it's a subtle issue. 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->set_type(type); > I don't have a "special value" or field to indicate if the value is set on What you've done here in the latest version (14) looks good. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.h File src/kudu/util/decimal_util.h: http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.h@28 PS14, Line 28: static const int8_t MAX_DECIMAL32_PRECISION = 9; the constants should be in the kMaxDecimal32Precision format, per our style guide. All uppercase is reserved for macros. 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) { Can we use the gutil fast pretty printers instead of this? Seems better to consolidate there, all things being equal. -- 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 19:17:21 +0000 Gerrit-HasComments: Yes
