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 16: (21 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: > Update this, since the for-loop is striding by 100. This is still true. The values are scaled up by 100 because of the scale 2. Meaning 5050 = 50.50. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc@241 PS14, Line 241: values.push_back(-9998); > 0 should already be added by the for-loop above. Done 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 Todd had suggested I do this in a previous review. If we can't do this I just need to add the old constructor back while still leaving this new private one as well. 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. There isn't a default for precision, but their is one for scale. I updated this. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@351 PS14, Line 351: : /// 0.9999 and -0.9999 > it's not or, right? Just -0.9999 to 0.9999 Done 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 Currently the scale must exactly match the columns scale when being used in a predicate. A follow up patch may allow some coercion. I am not sure adding a doc here makes sense though, given this KuduValue class could be used various ways. 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: > Just for my own understanding, is this coercion possible when scale < type_ This coercion is possible when I can make the values scale match the columns scale. The easiest and most common way will be when val.scale < col.scale and I can increase the value without being too large for the storage type. If col.scale = 3 and val.scale = 2. I can take the integer value * 10 an set val.scale = 3 as long as the value still fits in the storage type. I can reduce the scale as long as there are trailing zeros that I can truncate. Though I don't suspect that will be very common. http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@244 PS14, Line 244: return Status::InvalidArgument( > May want to put this check before the scale check, since I'd expect to get Done http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@248 PS14, Line 248: > Shouldn't this be checking against type_attributes.precision, not the min/m Yeah, perhaps. It depends on how strict we want to be with these KuduValues which are generally only used for predicates. As long as the scale matches and the value fits in the storage type we can "compare" values accurately. But technically you could create a predicate with a value that wouldn't fit in the column. 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@30 PS8, Line 30: : : : > did this algorithm get ported from somewhere? is there a tried and true one This was adapted from Impala's DecimalValue class. 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("binary_val", BINARY, true), > s/NULL/nullptr/g Done http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@215 PS14, Line 215: EXPECT_TRUE(row.IsColumnSet(4)); > This is kind of an odd representation. The column schema / type attributes This is coming from TypeInfo.AppendDebugStringForValue which doesn't have the context of the ColumnTypeAttributes. I can update KuduPartialRow::ToString() in a separate patch to use ColumnTypeAttributes. 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 <functional> > use <cstddef> Done 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: > Same here, should we check against the column precision? Yeah, thats a better check here. Will update. http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc File src/kudu/common/schema-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc@112 PS11, Line 112: ColumnTypeAttributes(18, 10)); > Does it make sense to cover DECIMAL128 here as well? Done http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc@155 PS11, Line 155: Schema schema_17_10({ col1, col_17_10 }, 1); > As I understand, we don't yet support decimal as a primary key. To express We actually support all decimals with a precision <= 18 in the primary key. This is because it maps server side to and INT32 or INT64. For precisions > 18 we need to add INT128 key support. I will add that immediately after this patch. 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 &kMinUnscaledDecimal64; > This also goes back to the precision from the column vs min/max for the buc Here we don't have the precision/scale information to work with since it rides outside of the type. http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/integration-tests/decimal-itest.cc File src/kudu/integration-tests/decimal-itest.cc: http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/integration-tests/decimal-itest.cc@159 PS11, Line 159: ASSERT_EQ("1234.00", DecimalToString(money, 2)); > Does it make sense to add a test which verifies that out-of-scale values ar Done 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 kMaxDecimal32Precision = 9; > the constants should be in the kMaxDecimal32Precision format, per our style Done 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: using std::string; > Can we use the gutil fast pretty printers instead of this? Seems better to You mean move this into gutil/strings/numbers.cc? http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/util/int128.h File src/kudu/util/int128.h: http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/util/int128.h@23 PS11, Line 23: <iostream> > Is it possible to use <iosfwd> here instead? Done -- 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: 16 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 <d...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 29 Jan 2018 21:05:14 +0000 Gerrit-HasComments: Yes
