[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8830 ) Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Reviewed-on: http://gerrit.cloudera.org:8080/8830 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin Reviewed-by: Grant Henke --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,356 insertions(+), 171 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved Grant Henke: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 33 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 32: Code-Review+2 Thanks everyone for the review. I am going to submit this. This is just the start of the feature. I will be adding a lot of follow on patches. If you have more feedback or review let me know and I can include it in a new smaller patch. -- 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: 32 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 02 Feb 2018 22:04:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Alexey Serbin 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 32: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc@190 PS28, Line 190: delete data_; : data_ = new Data(*other.data_); > All instances of CopyFrom in schema.cc behave this way. The = operator does SGTM: yep, it's better to keep in uniform in that case. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h@91 PS28, Line 91: public: > I was following "convention" of the other structs in this file. Should we d I tried to locate corresponding section in https://google.github.io/styleguide/cppguide.html, but didn't see anything. It's just a nit and if it's inline with the rest of the stuff, it's perfectly fine with me. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc File src/kudu/integration-tests/decimal-itest.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@55 PS28, Line 55: eName = "decimal-ta > Further down I set num_replicas to kNumServers. Ah, ok -- it seems I missed it. -- 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: 32 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 02 Feb 2018 21:58:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#32). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,356 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/32 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 32 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#30). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,355 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/30 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 30 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 28: (8 comments) http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.h@244 PS28, Line 244: /// This constructor is private because clients should use the Builder API. : /// : /// @param [in] name : /// The name of the column. : /// @param [in] type : /// The type of the column. : /// @param [in] is_nullable : /// Whether the column is nullable. : /// @param [in] default_value : /// Default value for the column. : /// @param [in] attributes : /// Column storage attributes. > nit: usually we don't document private methods since they are not accessibl Done http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc@190 PS28, Line 190: delete data_; : data_ = new Data(*other.data_); > What if other == *this? All instances of CopyFrom in schema.cc behave this way. The = operator does have the check before using it. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h@91 PS28, Line 91: public: > nit: it's public by default in struct, no need to specify explicitly I was following "convention" of the other structs in this file. Should we define this as style rule? http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc File src/kudu/integration-tests/decimal-itest.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@52 PS28, Line 52: const int kNumServers = 3; > nit: it's 3 tablet server by default, you could drop this. Further down I set num_replicas to kNumServers. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@55 PS28, Line 55: {}, {}, kNumServers > nit: I think it's possible to drop all this and use the parameters 'by defa Further down I set num_replicas to kNumServers. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@98 PS28, Line 98: client_->OpenTable(kTableName, &table); > nit: ASSERT_OK ? Done http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@130 PS28, Line 130: ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT)); > nit: it's possible to drop this since SetFaultTolerant() set the read mode Done http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@172 PS28, Line 172: } > nit: would it make sense to add an 'erroneous case' trying to read decimal 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: 28 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 02 Feb 2018 20:35:36 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#29). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,355 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/29 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 29 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Alexey Serbin 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 28: (8 comments) LGTM, just a few nits. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.h@244 PS28, Line 244: /// This constructor is private because clients should use the Builder API. : /// : /// @param [in] name : /// The name of the column. : /// @param [in] type : /// The type of the column. : /// @param [in] is_nullable : /// Whether the column is nullable. : /// @param [in] default_value : /// Default value for the column. : /// @param [in] attributes : /// Column storage attributes. nit: usually we don't document private methods since they are not accessible via client API. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc@190 PS28, Line 190: delete data_; : data_ = new Data(*other.data_); What if other == *this? http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h@91 PS28, Line 91: public: nit: it's public by default in struct, no need to specify explicitly http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc File src/kudu/integration-tests/decimal-itest.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@52 PS28, Line 52: const int kNumServers = 3; nit: it's 3 tablet server by default, you could drop this. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@55 PS28, Line 55: {}, {}, kNumServers nit: I think it's possible to drop all this and use the parameters 'by default' http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@98 PS28, Line 98: client_->OpenTable(kTableName, &table); nit: ASSERT_OK ? http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@130 PS28, Line 130: ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT)); nit: it's possible to drop this since SetFaultTolerant() set the read mode to READ_AT_SNAPSHOT 'automagically'. http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@172 PS28, Line 172: } nit: would it make sense to add an 'erroneous case' trying to read decimal as, say, float/double/string/ 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: 28 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 02 Feb 2018 19:25:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 28: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978 PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) { > The follow up patch to support int128 keys is actually required to support OK sounds good! I'm fine with landing this as-is as long as there's a path forwards. -- 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: 28 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 02 Feb 2018 18:08:15 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 28: (1 comment) http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978 PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) { > Sorry to keep harping on this, but I still feel we may be missing some impo The follow up patch to support int128 keys is actually required to support this tests using decimal128 because the predicates eventually call key_util::IncrementCell. I have that patch almost ready to push where I also update this test to use DECIMAL128. I will publish it shortly. I also have a separate patch increasing generic INT128 test coverage that I will push today too. -- 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: 28 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 02 Feb 2018 17:59:55 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 28: (1 comment) http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978 PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) { Sorry to keep harping on this, but I still feel we may be missing some important predicate coverage. IIUC this is only covering Decimal32, right? I'd like to at least see Decimal128 coverage as well, since that's a completely new physical type. In the follow up patch that does decimal primary keys we should make sure we have predicate coverage over decimal PKs as well, since the predicate optimization over PKs is extra special. -- 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: 28 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 02 Feb 2018 17:42:51 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#28). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,362 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/28 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 28 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#27). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,359 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/27 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 27 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#26). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,341 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/26 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 26 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#25). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 33 files changed, 1,340 insertions(+), 172 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/25 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 25 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#24). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,339 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/24 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 24 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#23). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 33 files changed, 1,341 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/23 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 23 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#22). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,339 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/22 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 22 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 14: (5 comments) http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc File src/kudu/common/partial_row-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc@227 PS20, Line 227: // the copy constructor. > This error message may be clearer if it printed the scaled value instead of Done 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; > Take a look at column_predicate-test for examples of type specific boundary I added a few tests in column_predicate-test but also simplified this code not to include the decimal "max" values but instead stick to the physical type maximums. In a follow up patch I will update places we use IsMinValue() and IsMaxValue() to take the ColumnTypeAtrributes into consideration and calculate the "real" limits for decimal columns. Until then the predicates are less than optimal but still correct. http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h File src/kudu/util/decimal_util.h: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h@63 PS20, Line 63: > Is having a default scale a benefit here? I'd expect that if the scale is True. I will remove the default. http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc File src/kudu/util/decimal_util.cc: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@28 PS20, Line 28: // 38 digits, 1 extra leading zero, decimal point, > Needs test Done http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@32 PS20, Line 32: int128_t n = d < 0? -d : d; > I think a for loop would be more idiomatic here. 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: 14 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 31 Jan 2018 19:42:36 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#21). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 32 files changed, 1,337 insertions(+), 169 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/21 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 21 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 20: (4 comments) http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc File src/kudu/common/partial_row-test.cc: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc@227 PS20, Line 227: EXPECT_EQ("Invalid argument: value 100 out of range for decimal column 'decimal_val'", This error message may be clearer if it printed the scaled value instead of the unscaled value (1.00 vs 100). http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h File src/kudu/util/decimal_util.h: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h@63 PS20, Line 63: std::string DecimalToString(int128_t value, int8_t scale = kDefaultDecimalScale); Is having a default scale a benefit here? I'd expect that if the scale is unknown the caller should use int128 stringification instead. http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc File src/kudu/util/decimal_util.cc: http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@28 PS20, Line 28: int128_t MaxUnscaledDecimal(int8_t precision) { Needs test http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@32 PS20, Line 32: while (precision) { I think a for loop would be more idiomatic here. for (; precision > 0; precision--) -- 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: 20 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Jan 2018 22:34:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 14: (1 comment) 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; > Take a look at column_predicate-test for examples of type specific boundary I will definitely add more detailed tests and explanation around this. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Jan 2018 22:17:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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: (2 comments) 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@248 PS14, Line 248: if (decimal_val_ < min_val || decimal_val_ > max_val) { > I think I will make this strict too so everything starts strict. We can the SGTM 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; > The way I looked at this is that this was the min_value/max_value that coul Take a look at column_predicate-test for examples of type specific boundary checks that would be useful to have for decimal. I'd also like to see this extended for each class of decimal. We've been bitten a few times in the past by incomplete edge case coverage on predicates, which is why the tests are so extensive / verbose now. I think you're right that the current implementation should be fine, but I'd feel better with more coverage. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Jan 2018 22:13:51 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#20). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 30 files changed, 1,286 insertions(+), 169 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/20 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 20 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#19). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 31 files changed, 1,287 insertions(+), 170 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/19 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 19 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 17: (4 comments) 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. > Why not document that the scale has to match exactly then? Done 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@248 PS14, Line 248: > Thats fair. Probably worth a comment if you keep it as is. I think I will make this strict too so everything starts strict. We can then loosen up the rules with well thought out coercion. 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: > Probably indicates a testing gap. I added a quick test in the update but will add some more bounds tests. 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; > Yeah I understand that, but the min_value/max_value contract is that it sho The way I looked at this is that this was the min_value/max_value that could be stored in the type regardless of the type attributes. Any type attributes only restrict the range further. I looked at the predicate code and we use the min_value/max_value to optimize the predicates by converting range predicates into IsNotNull or Equality predicates. Since the precision would only reduce the value set this shouldn't be an issue. I have a predicate test that check boundary values and values between of a decimal value. What types of additional tests should I add? -- 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: 17 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Jan 2018 03:44:54 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#18). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 31 files changed, 1,287 insertions(+), 170 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/18 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 18 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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: (8 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 > This is still true. The values are scaled up by 100 because of the scale 2. d'oh! 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, > Todd had suggested I do this in a previous review. If we can't do this I ju OK fine by me then. 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. > Currently the scale must exactly match the columns scale when being used in Why not document that the scale has to match exactly then? 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@248 PS14, Line 248: if (decimal_val_ < min_val || decimal_val_ > max_val) { > Yeah, perhaps. It depends on how strict we want to be with these KuduValues Thats fair. Probably worth a comment if you keep it as is. 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@215 PS14, Line 215: EXPECT_EQ("decimal decimal_val=123456_D32", row.ToString()); > This is coming from TypeInfo.AppendDebugStringForValue which doesn't have t Yeah, this is going to be a very confusing output since it's unscaled and the _D32 suffix is unlike anything else. We do use this output in a few places, including the scans dashboard I think. 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; > Yeah, thats a better check here. Will update. Probably indicates a testing gap. 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; > Here we don't have the precision/scale information to work with since it ri Yeah I understand that, but the min_value/max_value contract is that it should return the min and max value, not some other value. There's a bunch of predicate code written with this assumption in mind, and I'm nervous that this is going to break it. I think it's worth adding more complete coverage of edge cases to column_predicate-test for the different bucket sizes to be sure. Also keep in mind we do special predicate optimizations just for PK columns. 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) { > You mean move this into gutil/strings/numbers.cc? disregard -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 29 Jan 2018 22:50:26 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#17). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 31 files changed, 1,294 insertions(+), 170 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/17 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 17 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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. and -0. > it's not or, right? Just -0. to 0. 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.c
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#16). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 31 files changed, 1,295 insertions(+), 170 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/16 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 16 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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. to 0. 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 use 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.
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#15). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 31 files changed, 1,283 insertions(+), 170 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/15 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 15 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#14). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 31 files changed, 1,288 insertions(+), 170 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/14 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 14 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#13). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 31 files changed, 1,280 insertions(+), 170 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/13 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 13 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#12). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/predicate-test.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_predicate.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/common.proto M src/kudu/common/partial_row-test.cc M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/decimal_util-test.cc A src/kudu/util/decimal_util.cc A src/kudu/util/decimal_util.h M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 31 files changed, 1,275 insertions(+), 170 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/12 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 12 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Alexey Serbin 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 11: (5 comments) 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: Does it make sense to cover DECIMAL128 here as well? http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc@155 PS11, Line 155: As I understand, we don't yet support decimal as a primary key. To express that restriction explicitly, does it make sense to add a test to make sure the schema with decimal type as the key reports an error appropriately? 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: Does it make sense to add a test which verifies that out-of-scale values are handled properly by SetDecimal()? http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/integration-tests/decimal-itest.cc@160 PS11, Line 160: } // namespace client Consider adding a test which verifies that boundary decimal values are written and read as expected (for all scale/precision ranges). 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: Is it possible to use here instead? -- 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: 11 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 21:16:35 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#11). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal.cc A src/kudu/common/decimal.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 27 files changed, 868 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/11 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 11 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#10). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal.cc A src/kudu/common/decimal.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 27 files changed, 867 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/10 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#9). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal.cc A src/kudu/common/decimal.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 27 files changed, 867 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/9 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 8: (21 comments) Still tweaking the decimal impl a bit and adding some tests, but pushing an update for review and discussion. 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? Right, I should clarify. What I mean is they are not serialized with each value. I will update this. 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? Yeah, I think we should. I want to do that in a separate patch since the base functionality block progress on the Impala side. I can remove it in a separate patch too if that helps. 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 AB Yes, this should be exported. I will add KUDU_EXPORT. I modeled this to match KuduColumnStorageAttributes which doesn't use PImpl. Do you know why? 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 u Oh right, even defaulted arguments aren't compatible. I can make remove (make it private) if thats the route we should take. 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? This was taken right from the Impala docs. I agree though, I think a more accurate term would be "fractional". 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? Should be between 1 and 0. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@343 PS8, Line 343: /// columns precision. > typo: column's Done 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 Done 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 Done 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 DE This was discussed some on the design doc, but we never settled for sure. I think a default scale of 0 is reasonable and fairly common. Happy to change to use that default. 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 t Absolutely. Will add that. 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 agreed. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@297 PS8, Line 297: KuduColumnTypeAttributes kuduColumnTypeAttributes = > nit: var name style Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@641 PS8, Line 641: typeAttrs > nit: style Done 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@53 PS8, Line 53:static const int32_t MIN_DECIMAL_PRECISION = 0; > see comment elsewhere about precision 0 Done 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 s Yeah, I commented that I was pretty conflicted about this too on one of Dan's reviews. Had I created this class with no reference at all I would definitely have gone that route. The only thing that made me leave off precision and scale was the Impala
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 9 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:99) * 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
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#8). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal.cc A src/kudu/common/decimal.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/all_types-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 25 files changed, 665 insertions(+), 140 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/8 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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 ca
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#7). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal.cc A src/kudu/common/decimal.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/all_types-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 25 files changed, 666 insertions(+), 140 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/7 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Alexey Serbin 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: (12 comments) It seems I took a quick look at PS1 some time ago. Will re-visit this week. http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/client/schema.h@64 PS5, Line 64: public: nit: alignment 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: : pre I don't think this is needed unless we want to protect against list-initialized constructors. http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@76 PS1, Line 76: ret drop http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@81 PS1, Line 81: ret drop http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@321 PS1, Line 321: /// Add the documented description for the precision parameter. http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@328 PS1, Line 328: /// Add the documented description for the scale parameter. 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? http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h File src/kudu/common/decimal_value.h: http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h@32 PS5, Line 32: nit: spacing http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h@43 PS5, Line 43: nit: by code guidelines, the indent should be 2 spaces. 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 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 comment about that. 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 -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 03 Jan 2018 19:57:40 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Adar Dembo 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: (4 comments) I only looked at the public API changes. 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: public: Nit: indentation http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@70 PS6, Line 70: explicit KuduColumnTypeAttributes(int32_t precision, int32_t scale) The 'explicit' keyword is only needed to avoid implicit conversions with single-arg constructors, no? http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@75 PS6, Line 75: const int32_t precision() const { These accessors are returning an int32_t 'copy', so what value does the 'const' keyword add (as in 'const int32_t ...')? http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@84 PS6, Line 84: private: Do you anticipate KuduColumnTypeAttributes evolving for other use cases in the future? If so, you should PIMPL the class: define a private Data nested class, store precision/scale within it, and have the public class merely store an allocated pointer to the nested class. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 03 Jan 2018 19:18:24 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
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(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 >(col_idx, static_cast(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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 21 Dec 2017 01:14:27 + Gerrit
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#6). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal_value.cc A src/kudu/common/decimal_value.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/all_types-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 25 files changed, 630 insertions(+), 140 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/6 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#5). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal_value.cc A src/kudu/common/decimal_value.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/all_types-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 25 files changed, 629 insertions(+), 139 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/5 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#4). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal_value.cc A src/kudu/common/decimal_value.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/all_types-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 25 files changed, 629 insertions(+), 139 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/4 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#3). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal_value.cc A src/kudu/common/decimal_value.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/all_types-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 25 files changed, 636 insertions(+), 139 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/3 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#2). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Note: I have 2 failing tests but want to push this to start reviews while I work on them. Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal_value.cc A src/kudu/common/decimal_value.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/all_types-itest.cc M src/kudu/util/int128.h 24 files changed, 623 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/2 -- 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: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8830 Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Note: I have 2 failing tests but want to push this to start reviews while I work on them. Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal_value.cc A src/kudu/common/decimal_value.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/all_types-itest.cc 23 files changed, 631 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/1 -- 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: newchange Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke