You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2017/12/13 17:15:44 UTC
[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 <gr...@gmail.com>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 03 Jan 2018 19:57:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8830 )
Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
......................................................................
Patch Set 8:
(23 comments)
I think this could do with a couple end-to-end tests from the client writing decimals of different precision and making sure they come back reasonably
http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@17
PS8, Line 17: stored
not stored? it must be stored in the metadata somewhere right?
http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@24
PS8, Line 24: int128 suffixes
perhaps we should guard them with a preprocessor check for c++11 instead?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@63
PS8, Line 63: class KuduColumnTypeAttributes {
should this be exported for users? If so I think we need to PIMPL it for ABI compatibility (I imagine this is where we might later add stuff like charsets for strings?)
If not exported then I think it belongs in schema-internal.h or somesuch
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@177
PS8, Line 177: /// @todo KUDU-809: make this hard-to-use constructor private.
: /// Clients should use the Builder API. Currently only the Python API
: /// uses this old API.
this has been deprecated for several releases now and I dont think python uses it anymore. Maybe we can remove it (make it private)? Changing its signature is equally as illegal as removing it.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@323
PS8, Line 323: floating-point
it's not floating point, right?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@328
PS8, Line 328: /// The precision must be between 0 and 38.
curious what a precision of 0 means... that can only store the value 0?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@343
PS8, Line 343: /// columns precision.
typo: column's
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@133
PS8, Line 133: return kudu::DECIMAL128;
nit: weird indentation
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@153
PS8, Line 153: case kudu::DECIMAL32 : return KuduColumnSchema::DECIMAL;
nit: extra space
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@266
PS8, Line 266: return Status::InvalidArgument("no scale provided for decimal column", data_->name);
is a default scale of 0 not reasonable? I seem to recall that sql allows DECIMAL(5) and that means 0 through 99999
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@285
PS8, Line 285: }
can we check that no precision/scale are specified for non-decimal column types? want to make sure someone doesn't try to do STRING(10) under some assumption this would produce a fixed-length or truncated string type (as in sql VARCHAR(10))
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@287
PS8, Line 287: int32_t precision = 0;
: if (data_->has_precision) {
: precision = data_->precision;
: }
think ternary would be easier to read here and below
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@297
PS8, Line 297: KuduColumnTypeAttributes kuduColumnTypeAttributes =
nit: var name style
Also you can just construct this like KuduColumnTypeAttributes attr(precision, scale) -- no need for the extra assignment
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@641
PS8, Line 641: typeAttrs
nit: style
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h
File src/kudu/common/decimal.h:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@49
PS8, Line 49: 999999999999999999) * 100) + 99; // 38 9's
well this is fun. have you tested that this works as expected and not getting secretly truncated somewhere along the line?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@53
PS8, Line 53: static const int32_t MIN_DECIMAL_PRECISION = 0;
> warning: invalid case style for global constant 'MIN_DECIMAL_PRECISION' [re
see comment elsewhere about precision 0
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@63
PS8, Line 63: explicit Decimal(int128_t s) :
I sort of feel like this Decimal instance should retain its precision and scale as a member and then check for compatibility in the appropriate spots. Otherwise it will be very easy to get incorrect results if you accidentally pass a non-matching decimal Value into something like a predicate, no?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc
File src/kudu/common/decimal.cc:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@25
PS8, Line 25: int last_char_idx = precision
nit: indentation off in this file
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@29
PS8, Line 29: string
no need to repeat type name
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@30
PS8, Line 30: // Start filling in the values in reverse order by taking the last digit
: // of the value. Use a positive value and worry about the sign later. At this
: // point the last_char_idx points to the string terminator.
:
did this algorithm get ported from somewhere? is there a tried and true one in sqlite or somesuch?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/types.h@558
PS8, Line 558: // to format it.
maybe we should still format it in such a way that it's clear the value being written is not actually a normal int? eg 12345/? or somesuch? or D12345? 12345D? '12345e?' ?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc@214
PS8, Line 214: pb->mutable_type_attributes()->set_precision(col_schema.type_attributes().precision);
maybe we should only serialize them in the case that it is decimal, to save space/deserialization cost?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc@264
PS8, Line 264: typeAttributes
style (also below)
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jan 2018 03:11:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 18:08:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jan 2018 22:13:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jan 2018 03:44:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8830 )
Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
......................................................................
Patch Set 14:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc@231
PS14, Line 231: // Returns a vector of decimal(4, 2) numbers from -50.50 (inclusive) to 50.50
Update this, since the for-loop is striding by 100.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc@241
PS14, Line 241: values.push_back(0);
0 should already be added by the for-loop above.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@256
PS14, Line 256: KuduColumnSchema(const std::string &name,
Unfortunately I don't think you can move/change this API in this way, even though it's deprecated.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@334
PS14, Line 334: represented by
'represented' is used twice in the sentence in slightly different ways, which may be confusing
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@343
PS14, Line 343: KuduColumnSpec* Precision(int8_t precision);
Is there a default value? If so doc it.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@351
PS14, Line 351: 0 and 0.999...
: /// or 0 and -0.999...
it's not or, right? Just -0.9999 to 0.9999
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h
File src/kudu/client/value.h:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h@63
PS14, Line 63: /// The decimal value's scale.
Is there a restriction that scale must be < the column's scale? If so add that note.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc
File src/kudu/client/value.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@221
PS14, Line 221: // TODO: Coerce when possible
Just for my own understanding, is this coercion possible when scale < type_attributes.scale?
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@244
PS14, Line 244: Substitute("invalid type $0 provided for column '$1' (expected DECIMAL)",
May want to put this check before the scale check, since I'd expect to get a type error instead of a scale error for a non-decimal column.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@248
PS14, Line 248: if (decimal_val_ < min_val || decimal_val_ > max_val) {
Shouldn't this be checking against type_attributes.precision, not the min/max for that size bucket?
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@43
PS14, Line 43: ColumnSchema("decimal_val", DECIMAL32, true, NULL, NULL,
s/NULL/nullptr/g
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@215
PS14, Line 215: EXPECT_EQ("decimal decimal_val=123456_D32", row.ToString());
This is kind of an odd representation. The column schema / type attributes are available, right? Can it be something like 'decimal(x, y) decimal_val=123456'?
http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc@20
PS15, Line 20: #include <string>
use <cstddef>
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row.cc@305
PS14, Line 305: int128_t min_val, max_val;
Same here, should we check against the column precision?
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h@564
PS14, Line 564: return &MIN_UNSCALED_DECIMAL64;
This also goes back to the precision from the column vs min/max for the bucket thing. This min_value/max_value is used for optimizing predicates, and this is going to play into it. I think what we're doing here is technically correct, but not optimal. We can discuss on chat, it's a subtle issue.
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/wire_protocol.cc@214
PS6, Line 214: pb->set_type(type);
> I don't have a "special value" or field to indicate if the value is set on
What you've done here in the latest version (14) looks good.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.h
File src/kudu/util/decimal_util.h:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.h@28
PS14, Line 28: static const int8_t MAX_DECIMAL32_PRECISION = 9;
the constants should be in the kMaxDecimal32Precision format, per our style guide. All uppercase is reserved for macros.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.cc
File src/kudu/util/decimal_util.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.cc@27
PS14, Line 27: string DecimalToString(int128_t d, int8_t scale) {
Can we use the gutil fast pretty printers instead of this? Seems better to consolidate there, all things being equal.
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Jan 2018 19:17:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Reviewed-by: Grant Henke <gr...@gmail.com>
---
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 03 Jan 2018 19:18:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jan 2018 22:17:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Jan 2018 22:50:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:35:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 Implementation. The comment on Impala's DecimalValue is, "The decimal does not store its precision and scale since we'd like to keep the storage as small as possible".
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc
File src/kudu/common/decimal.cc:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@25
PS8, Line 25: int last_char_idx = precision
> nit: indentation off in this file
Done
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@29
PS8, Line 29: string
> no need to repeat type name
Done
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/types.h@558
PS8, Line 558: // to format it.
> maybe we should still format it in such a way that it's clear the value bei
Yeah, I think thats a good idea. I could add _D32, _D64, and _D128 to the end of the numbers. That gives the added benefit of knowing the storage size.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc@214
PS8, Line 214: pb->mutable_type_attributes()->set_precision(col_schema.type_attributes().precision);
> maybe we should only serialize them in the case that it is decimal, to save
Good idea. Will do.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc@264
PS8, Line 264: typeAttributes
> style (also below)
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: 8
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 16 Jan 2018 19:17:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:58:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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<int128_t>(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<TypeTraits<DECIMAL32> >(col_idx, static_cast<int32_t>(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 <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 21 Dec 2017 01:14:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 1000000 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 (10000.00 vs 1000000).
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jan 2018 22:34:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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: <iostream>
Is it possible to use <iosfwd> 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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 21:16:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:04:18 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 19:25:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 31 Jan 2018 19:42:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/8830 )
Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
......................................................................
Patch Set 16:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc@231
PS14, Line 231:
> Update this, since the for-loop is striding by 100.
This is still true. The values are scaled up by 100 because of the scale 2. Meaning 5050 = 50.50.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/predicate-test.cc@241
PS14, Line 241: values.push_back(-9998);
> 0 should already be added by the for-loop above.
Done
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@256
PS14, Line 256: KuduColumnSchema(const std::string &name,
> Unfortunately I don't think you can move/change this API in this way, even
Todd had suggested I do this in a previous review. If we can't do this I just need to add the old constructor back while still leaving this new private one as well.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@343
PS14, Line 343: KuduColumnSpec* Precision(int8_t precision);
> Is there a default value? If so doc it.
There isn't a default for precision, but their is one for scale. I updated this.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@351
PS14, Line 351:
: /// 0.9999 and -0.9999
> it's not or, right? Just -0.9999 to 0.9999
Done
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h
File src/kudu/client/value.h:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h@63
PS14, Line 63: /// The decimal value's scale.
> Is there a restriction that scale must be < the column's scale? If so add
Currently the scale must exactly match the columns scale when being used in a predicate. A follow up patch may allow some coercion. I am not sure adding a doc here makes sense though, given this KuduValue class could be used various ways.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc
File src/kudu/client/value.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@221
PS14, Line 221:
> Just for my own understanding, is this coercion possible when scale < type_
This coercion is possible when I can make the values scale match the columns scale. The easiest and most common way will be when val.scale < col.scale and I can increase the value without being too large for the storage type.
If col.scale = 3 and val.scale = 2. I can take the integer value * 10 an set val.scale = 3 as long as the value still fits in the storage type.
I can reduce the scale as long as there are trailing zeros that I can truncate. Though I don't suspect that will be very common.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@244
PS14, Line 244: return Status::InvalidArgument(
> May want to put this check before the scale check, since I'd expect to get
Done
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@248
PS14, Line 248:
> Shouldn't this be checking against type_attributes.precision, not the min/m
Yeah, perhaps. It depends on how strict we want to be with these KuduValues which are generally only used for predicates.
As long as the scale matches and the value fits in the storage type we can "compare" values accurately. But technically you could create a predicate with a value that wouldn't fit in the column.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc
File src/kudu/common/decimal.cc:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@30
PS8, Line 30:
:
:
:
> did this algorithm get ported from somewhere? is there a tried and true one
This was adapted from Impala's DecimalValue class.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@43
PS14, Line 43: ColumnSchema("binary_val", BINARY, true),
> s/NULL/nullptr/g
Done
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@215
PS14, Line 215: EXPECT_TRUE(row.IsColumnSet(4));
> This is kind of an odd representation. The column schema / type attributes
This is coming from TypeInfo.AppendDebugStringForValue which doesn't have the context of the ColumnTypeAttributes. I can update KuduPartialRow::ToString() in a separate patch to use ColumnTypeAttributes.
http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc@20
PS15, Line 20: #include <functional>
> use <cstddef>
Done
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row.cc@305
PS14, Line 305:
> Same here, should we check against the column precision?
Yeah, thats a better check here. Will update.
http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc
File src/kudu/common/schema-test.cc:
http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc@112
PS11, Line 112: ColumnTypeAttributes(18, 10));
> Does it make sense to cover DECIMAL128 here as well?
Done
http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc@155
PS11, Line 155: Schema schema_17_10({ col1, col_17_10 }, 1);
> As I understand, we don't yet support decimal as a primary key. To express
We actually support all decimals with a precision <= 18 in the primary key. This is because it maps server side to and INT32 or INT64. For precisions > 18 we need to add INT128 key support. I will add that immediately after this patch.
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h@564
PS14, Line 564: return &kMinUnscaledDecimal64;
> This also goes back to the precision from the column vs min/max for the buc
Here we don't have the precision/scale information to work with since it rides outside of the type.
http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/integration-tests/decimal-itest.cc
File src/kudu/integration-tests/decimal-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/integration-tests/decimal-itest.cc@159
PS11, Line 159: ASSERT_EQ("1234.00", DecimalToString(money, 2));
> Does it make sense to add a test which verifies that out-of-scale values ar
Done
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.h
File src/kudu/util/decimal_util.h:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.h@28
PS14, Line 28: static const int8_t kMaxDecimal32Precision = 9;
> the constants should be in the kMaxDecimal32Precision format, per our style
Done
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.cc
File src/kudu/util/decimal_util.cc:
http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.cc@27
PS14, Line 27: using std::string;
> Can we use the gutil fast pretty printers instead of this? Seems better to
You mean move this into gutil/strings/numbers.cc?
http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/util/int128.h
File src/kudu/util/int128.h:
http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/util/int128.h@23
PS11, Line 23: <iostream>
> Is it possible to use <iosfwd> here instead?
Done
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 16
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Jan 2018 21:05:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 17:42:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 17:59:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/8830 )
Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
......................................................................
Patch Set 1:
(25 comments)
I need to tweak the decimal class a bit, but I updated based on the reviews.
http://gerrit.cloudera.org:8080/#/c/8830/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8830/6//COMMIT_MSG@24
PS6, Line 24: internal types are represented and stored as
> Could we if/def them out?
I wasn't sure exactly how to do that in a way that wouldn't break the client. I really want to add this back but should do so after the initial patch.
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@64
PS6, Line 64: class KuduColumnTypeAttributes {
> Nit: indentation
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@70
PS6, Line 70:
> The 'explicit' keyword is only needed to avoid implicit conversions with si
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@75
PS6, Line 75: /// @return Precision for the column type.
> These accessors are returning an int32_t 'copy', so what value does the 'co
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@84
PS6, Line 84:
> Do you anticipate KuduColumnTypeAttributes evolving for other use cases in
The only other attribute I can see being added as of now is "size". Though precision could be "reused" for that if we want.
That would cover CHAR(size), VARCHAR(size), TIMESTAMP(precision) types if we want to add them in the future.
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@320
PS6, Line 320: ///
> Add a note about the valid value ranges here and below.
Done
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@71
PS1, Line 71: explicit
> I don't think this is needed unless we want to protect against list-initial
Done
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@76
PS1, Line 76: const
> drop
Done
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@81
PS1, Line 81: const
> drop
Done
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@321
PS1, Line 321: /// Clients must specify a precision for decimal columns.
> Add the documented description for the precision parameter.
Done
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@328
PS1, Line 328: /// Clients must specify a scale for decimal columns.
> Add the documented description for the scale parameter.
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.cc@261
PS6, Line 261: if (data_->type == KuduColumnSchema::DECIMAL && !data_->has_scale) {
> I think this should be checking the scale/precision are within range, other
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/value-internal.h
File src/kudu/client/value-internal.h:
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/value-internal.h@48
PS6, Line 48: Slice slice_val_;
> just curious - do you know why this isn't in the union? I'm mildly concerne
slice_val_ can't be added to the union because it has a non-trivial default constructor. I tried re-arranging the fields but regardless the sizeof data is 48.
http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto@54
PS5, Line 54: DECIMAL32 = 17;
> nit: could you add a comment to explain why 15 and 16 are skipped?
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h
File src/kudu/common/decimal_value.h:
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@31
PS6, Line 31: xported head
> bikesheddy, but I'd mildly prefer plain 'Decimal' for this class name. If
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@32
PS6, Line 32: #include "kudu/client/stubs.h"
> indentation
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@38
PS6, Line 38:
> 'can be' here and below
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@57
PS6, Line 57:
> I'd err on leaving this out of the public API unless there's a really compe
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/decimal_value.h@66
PS6, Line 66: // Returns a string representation of the DecimalValue with a given precision
> I somewhat expected there to be basic math functions (at least add/subtract
yeah, I think this should be a follow up. Decimal math functions can be tricky and the main motivation for decimal support is for integrations as opposed to direct use. Those integrations already have their own math implementations.
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/partial_row.cc@304
PS6, Line 304: case DECIMAL32:
> We should consider doing bounds checks here, at least in debug mode.
Yeah, this is where I am not 100% sure what I/we should do. I could leave DecimalValue without precision/scale as it is now and validate the value "fits" here. Or I could have a precision/scale on DecimalValue and validate it matches the columns precision/scale here. Then all bounds checks would be a DecimalValue creation time.
I just can't decide how "thin" DecimalValue should be.
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@50
PS1, Line 50: DataType type) const {
> nit: misaligned line for the second parameter
Done
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@57
PS1, Line 57: true
> Why true, not false? If there is some specific reason, please add a commen
Done
http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@125
PS1, Line 125: return strings::Substitute("$0$1 $2",
> nit: missed space
This is on purpose.This is so the result is decimal(1, 2) instead of decimal (1, 2).
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/schema.cc@50
PS6, Line 50: DataType type) const {
> indent
Done
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/common/wire_protocol.cc@214
PS6, Line 214: pb->mutable_type_attributes()->set_precision(col_schema.type_attributes().precision);
> Is it possible to if guard these as well? That way the scale/precision won
I don't have a "special value" or field to indicate if the value is set on ColumnTypeAttributes. I could make that change, but I took the route of just defaulting to 0 so far. I don't feel too strongly either way.
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 09 Jan 2018 18:38:18 +0000
Gerrit-HasComments: Yes