[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Grant Henke has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Reviewed-on: http://gerrit.cloudera.org:8080/8830
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
Reviewed-by: Grant Henke 
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,356 insertions(+), 171 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved
  Grant Henke: Looks good to me, approved

--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 33
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 32: Code-Review+2

Thanks everyone for the review. I am going to submit this.

This is just the start of the feature. I will be adding a lot of follow on 
patches. If you have more feedback or review let me know and I can include it 
in a new smaller patch.


--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 32
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:04:18 +
Gerrit-HasComments: No

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 32: Code-Review+2

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc@190
PS28, Line 190:   delete data_;
  :   data_ = new Data(*other.data_);
> All instances of CopyFrom in schema.cc behave this way. The = operator does
SGTM: yep, it's better to keep in uniform in that case.


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h
File src/kudu/common/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h@91
PS28, Line 91:  public:
> I was following "convention" of the other structs in this file. Should we d
I tried to locate corresponding section in 
https://google.github.io/styleguide/cppguide.html, but didn't see anything.

It's just a nit and if it's inline with the rest of the stuff, it's perfectly 
fine with me.


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc
File src/kudu/integration-tests/decimal-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@55
PS28, Line 55: eName = "decimal-ta
> Further down I set num_replicas to kNumServers.
Ah, ok -- it seems I missed it.



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 32
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:58:12 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#32).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,356 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/32
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 32
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#30).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,355 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/30
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 30
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

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, );
> nit: ASSERT_OK ?
Done


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@130
PS28, Line 130:   ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
> nit: it's possible to drop this since SetFaultTolerant() set the read mode
Done


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@172
PS28, Line 172: }
> nit: would it make sense to add an 'erroneous case' trying to read decimal
Done



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:35:36 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#29).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,355 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/29
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 29
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Alexey Serbin (Code Review)]

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, );
nit: ASSERT_OK ?


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@130
PS28, Line 130:   ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
nit: it's possible to drop this since SetFaultTolerant() set the read mode to 
READ_AT_SNAPSHOT 'automagically'.


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@172
PS28, Line 172: }
nit: would it make sense to add an 'erroneous case' trying to read decimal as, 
say, float/double/string/ etc.?



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 19:25:00 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 28: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978
PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) {
> The follow up patch to support int128 keys is actually required to support
OK sounds good!  I'm fine with landing this as-is as long as there's a path 
forwards.



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 18:08:15 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 28:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978
PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) {
> Sorry to keep harping on this, but I still feel we may be missing some impo
The follow up patch to support int128 keys is actually required to support this 
tests using decimal128 because the predicates eventually call 
key_util::IncrementCell. I have that patch almost ready to push where I also 
update this test to use DECIMAL128. I will publish it shortly.

I also have a separate patch increasing generic INT128 test coverage that I 
will push today too.



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 17:59:55 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 28:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978
PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) {
Sorry to keep harping on this, but I still feel we may be missing some 
important predicate coverage.  IIUC this is only covering Decimal32, right?  
I'd like to at least see Decimal128 coverage as well, since that's a completely 
new physical type.

In the follow up patch that does decimal primary keys we should make sure we 
have predicate coverage over decimal PKs as well, since the predicate 
optimization over PKs is extra special.



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 17:42:51 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#28).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,362 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/28
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#27).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,359 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/27
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 27
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#26).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,341 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/26
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 26
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#25).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
33 files changed, 1,340 insertions(+), 172 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/25
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 25
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#24).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,339 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/24
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 24
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#22).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,339 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/22
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 22
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

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 _UNSCALED_DECIMAL64;
> Take a look at column_predicate-test for examples of type specific boundary
I added a few tests in column_predicate-test but also simplified this code not 
to include the decimal "max" values but instead stick to the physical type 
maximums.

In a follow up patch I will update places we use IsMinValue() and IsMaxValue() 
to take the ColumnTypeAtrributes into consideration and calculate the "real" 
limits for decimal columns. Until then the predicates are less than optimal but 
still correct.


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h
File src/kudu/util/decimal_util.h:

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h@63
PS20, Line 63:
> Is having a default scale a benefit here?  I'd expect that if the scale is
True. I will remove the default.


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc
File src/kudu/util/decimal_util.cc:

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@28
PS20, Line 28:   // 38 digits, 1 extra leading zero, decimal point,
> Needs test
Done


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@32
PS20, Line 32:   int128_t n = d < 0? -d : d;
> I think a for loop would be more idiomatic here.
Done



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 31 Jan 2018 19:42:36 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#21).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,337 insertions(+), 169 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/21
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 21
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 20:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc@227
PS20, Line 227:   EXPECT_EQ("Invalid argument: value 100 out of range for 
decimal column 'decimal_val'",
This error message may be clearer if it printed the scaled value instead of the 
unscaled value (1.00 vs 100).


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h
File src/kudu/util/decimal_util.h:

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h@63
PS20, Line 63:   std::string DecimalToString(int128_t value, int8_t scale = 
kDefaultDecimalScale);
Is having a default scale a benefit here?  I'd expect that if the scale is 
unknown the caller should use int128 stringification instead.


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc
File src/kudu/util/decimal_util.cc:

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@28
PS20, Line 28: int128_t MaxUnscaledDecimal(int8_t precision) {
Needs test


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@32
PS20, Line 32:   while (precision) {
I think a for loop would be more idiomatic here.

for (; precision > 0; precision--)



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 20
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 30 Jan 2018 22:34:57 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

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 _UNSCALED_DECIMAL64;
> Take a look at column_predicate-test for examples of type specific boundary
I will definitely add more detailed tests and explanation around this.



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 30 Jan 2018 22:17:57 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Dan Burkert (Code Review)]

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 _UNSCALED_DECIMAL64;
> The way I looked at this is that this was the min_value/max_value that coul
Take a look at column_predicate-test for examples of type specific boundary 
checks that would be useful to have for decimal.  I'd also like to see this 
extended for each class of decimal.  We've been bitten a few times in the past 
by incomplete edge case coverage on predicates, which is why the tests are so 
extensive / verbose now.

I think you're right that the current implementation should be fine, but I'd 
feel better with more coverage.



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 30 Jan 2018 22:13:51 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#20).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
30 files changed, 1,286 insertions(+), 169 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/20
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 20
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#19).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
31 files changed, 1,287 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/19
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 19
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

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 
> Yeah I understand that, but the min_value/max_value contract is that it sho
The way I looked at this is that this was the min_value/max_value that could be 
stored in the type regardless of the type attributes. Any type attributes only 
restrict the range further.

I looked at the predicate code and we use the min_value/max_value to optimize 
the predicates by converting range predicates into IsNotNull or Equality 
predicates. Since the precision would only reduce the value set this shouldn't 
be an issue.

I have a predicate test that check boundary values and values between of a 
decimal value. What types of additional tests should I add?



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 17
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 30 Jan 2018 03:44:54 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#18).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
31 files changed, 1,287 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/18
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 18
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Dan Burkert (Code Review)]

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 ,
> 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 _UNSCALED_DECIMAL64;
> Here we don't have the precision/scale information to work with since it ri
Yeah I understand that, but the min_value/max_value contract is that it should 
return the min and max value, not some other value.  There's a bunch of 
predicate code written with this assumption in mind, and I'm nervous that this 
is going to break it.  I think it's worth adding more complete coverage of edge 
cases to column_predicate-test for the different bucket sizes to be sure.  Also 
keep in mind we do special predicate optimizations just for PK columns.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.cc
File src/kudu/util/decimal_util.cc:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/util/decimal_util.cc@27
PS14, Line 27: string DecimalToString(int128_t d, int8_t scale) {
> You mean move this into gutil/strings/numbers.cc?
disregard



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 29 Jan 2018 22:50:26 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#17).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
31 files changed, 1,294 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/17
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 17
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

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 ,
> Unfortunately I don't think you can move/change this API in this way, even
Todd had suggested I do this in a previous review. If we can't do this I just 
need to add the old constructor back while still leaving this new private one 
as well.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@343
PS14, Line 343:   KuduColumnSpec* Precision(int8_t precision);
> Is there a default value?  If so doc it.
There isn't a default for precision, but their is one for scale. I updated this.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@351
PS14, Line 351:
  :   /// 0. and -0.
> it's not or, right?  Just -0. to 0.
Done


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h
File src/kudu/client/value.h:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h@63
PS14, Line 63:   ///   The decimal value's scale.
> Is there a restriction that scale must be < the column's scale?  If so add
Currently the scale must exactly match the columns scale when being used in a 
predicate. A follow up patch may allow some coercion. I am not sure adding a 
doc here makes sense though, given this KuduValue class could be used various 
ways.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc
File src/kudu/client/value.cc:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@221
PS14, Line 221:
> Just for my own understanding, is this coercion possible when scale < type_
This coercion is possible when I can make the values scale match the columns 
scale. The easiest and most common way will be when val.scale < col.scale and I 
can increase the value without being too large for the storage type.

If col.scale = 3 and val.scale = 2. I can take the integer value * 10 an set 
val.scale = 3 as long as the value still fits in the storage type.

I can reduce the scale as long as there are trailing zeros that I can truncate. 
Though I don't suspect that will be very common.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@244
PS14, Line 244: return Status::InvalidArgument(
> May want to put this check before the scale check, since I'd expect to get
Done


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@248
PS14, Line 248:
> Shouldn't this be checking against type_attributes.precision, not the min/m
Yeah, perhaps. It depends on how strict we want to be with these KuduValues 
which are generally only used for predicates.

As long as the scale matches and the value fits in the storage type we can 
"compare" values accurately. But technically you could create a predicate with 
a value that wouldn't fit in the column.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc
File src/kudu/common/decimal.cc:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@30
PS8, Line 30:
:
:
:
> did this algorithm get ported from somewhere? is there a tried and true one
This was adapted from Impala's DecimalValue class.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@43
PS14, Line 43: ColumnSchema("binary_val", BINARY, true),
> s/NULL/nullptr/g
Done


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@215
PS14, Line 215:   EXPECT_TRUE(row.IsColumnSet(4));
> This is kind of an odd representation.  The column schema / type attributes
This is coming from TypeInfo.AppendDebugStringForValue which doesn't have the 
context of the ColumnTypeAttributes. I can update KuduPartialRow::ToString() in 
a separate patch to use ColumnTypeAttributes.


http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#16).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
31 files changed, 1,295 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/16
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 16
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Dan Burkert (Code Review)]

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 ,
Unfortunately I don't think you can move/change this API in this way, even 
though it's deprecated.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@334
PS14, Line 334: represented by
'represented' is used twice in the sentence in slightly different ways, which 
may be confusing


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@343
PS14, Line 343:   KuduColumnSpec* Precision(int8_t precision);
Is there a default value?  If so doc it.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/schema.h@351
PS14, Line 351: 0 and 0.999...
  :   /// or 0 and -0.999...
it's not or, right?  Just -0. to 0.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h
File src/kudu/client/value.h:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.h@63
PS14, Line 63:   ///   The decimal value's scale.
Is there a restriction that scale must be < the column's scale?  If so add that 
note.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc
File src/kudu/client/value.cc:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@221
PS14, Line 221:   // TODO: Coerce when possible
Just for my own understanding, is this coercion possible when scale < 
type_attributes.scale?


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@244
PS14, Line 244:   Substitute("invalid type $0 provided for column '$1' 
(expected DECIMAL)",
May want to put this check before the scale check, since I'd expect to get a 
type error instead of a scale error for a non-decimal column.


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/client/value.cc@248
PS14, Line 248:   if (decimal_val_ < min_val || decimal_val_ > max_val) {
Shouldn't this be checking against type_attributes.precision, not the min/max 
for that size bucket?


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@43
PS14, Line 43: ColumnSchema("decimal_val", DECIMAL32, true, 
NULL, NULL,
s/NULL/nullptr/g


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row-test.cc@215
PS14, Line 215:   EXPECT_EQ("decimal decimal_val=123456_D32", row.ToString());
This is kind of an odd representation.  The column schema / type attributes are 
available, right?  Can it be something like 'decimal(x, y) decimal_val=123456'?


http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/15/src/kudu/common/partial_row-test.cc@20
PS15, Line 20: #include 
use 


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/partial_row.cc@305
PS14, Line 305:   int128_t min_val, max_val;
Same here, should we check against the column precision?


http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h
File src/kudu/common/types.h:

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h@564
PS14, Line 564: return _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.

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#15).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
31 files changed, 1,283 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/15
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#14).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
31 files changed, 1,288 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/14
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#13).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
31 files changed, 1,280 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/13
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 13
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#12).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
31 files changed, 1,275 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/12
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 12
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 11:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc
File src/kudu/common/schema-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc@112
PS11, Line 112:
Does it make sense to cover DECIMAL128 here as well?


http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/common/schema-test.cc@155
PS11, Line 155:
As I understand, we don't yet support decimal as a primary key.  To express 
that restriction explicitly, does it make sense to add a test to make sure the 
schema with decimal type as the key reports an error appropriately?


http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/integration-tests/decimal-itest.cc
File src/kudu/integration-tests/decimal-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/integration-tests/decimal-itest.cc@159
PS11, Line 159:
Does it make sense to add a test which verifies that out-of-scale values are 
handled properly by SetDecimal()?


http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/integration-tests/decimal-itest.cc@160
PS11, Line 160: } // namespace client
Consider adding a test which verifies that boundary decimal values are written 
and read as expected (for all scale/precision ranges).


http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/util/int128.h
File src/kudu/util/int128.h:

http://gerrit.cloudera.org:8080/#/c/8830/11/src/kudu/util/int128.h@23
PS11, Line 23: 
Is it possible to use  here instead?



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 17 Jan 2018 21:16:35 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#11).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal.cc
A src/kudu/common/decimal.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
27 files changed, 868 insertions(+), 164 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/11
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#10).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal.cc
A src/kudu/common/decimal.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
27 files changed, 867 insertions(+), 164 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/10
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#9).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because they break the
client by using C++11 features. They may be added back in a
different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal.cc
A src/kudu/common/decimal.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
27 files changed, 867 insertions(+), 164 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/9
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 8:

(21 comments)

Still tweaking the decimal impl a bit and adding some tests, but pushing an 
update for review and discussion.

http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@17
PS8, Line 17: stored
> not stored? it must be stored in the metadata somewhere right?
Right, I should clarify. What I mean is they are not serialized with each 
value. I will update this.


http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@24
PS8, Line 24: int128 suffixes
> perhaps we should guard them with a preprocessor check for c++11 instead?
Yeah, I think we should. I want to do that in a separate patch since the base 
functionality block progress on the Impala side. I can remove it in a separate 
patch too if that helps.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@63
PS8, Line 63: class KuduColumnTypeAttributes {
> should this be exported for users? If so I think we need to PIMPL it for AB
Yes, this should be exported. I will add KUDU_EXPORT. I modeled this to match 
KuduColumnStorageAttributes which doesn't use PImpl. Do you know why?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@177
PS8, Line 177:   /// @todo KUDU-809: make this hard-to-use constructor private.
 :   ///   Clients should use the Builder API. Currently only the 
Python API
 :   ///   uses this old API.
> this has been deprecated for several releases now and I dont think python u
Oh right, even defaulted arguments aren't compatible.
I can make remove (make it private) if thats the route we should take.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@323
PS8, Line 323: floating-point
> it's not floating point, right?
This was taken right from the Impala docs.
I agree though, I think a more accurate term would be "fractional".


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@328
PS8, Line 328:   /// The precision must be between 0 and 38.
> curious what a precision of 0 means... that can only store the value 0?
Should be between 1 and 0.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@343
PS8, Line 343:   /// columns precision.
> typo: column's
Done


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@133
PS8, Line 133:   return kudu::DECIMAL128;
> nit: weird indentation
Done


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@153
PS8, Line 153: case kudu::DECIMAL32 : return KuduColumnSchema::DECIMAL;
> nit: extra space
Done


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@266
PS8, Line 266:   return Status::InvalidArgument("no scale provided for 
decimal column", data_->name);
> is a default scale of 0 not reasonable? I seem to recall that sql allows DE
This was discussed some on the design doc, but we never settled for sure. I 
think a default scale of 0 is reasonable and fairly common. Happy to change to 
use that default.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@285
PS8, Line 285:   }
> can we check that no precision/scale are specified for non-decimal column t
Absolutely. Will add that.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@287
PS8, Line 287:   int32_t precision = 0;
 :   if (data_->has_precision) {
 : precision = data_->precision;
 :   }
> think ternary would be easier to read here and below
agreed.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@297
PS8, Line 297:   KuduColumnTypeAttributes kuduColumnTypeAttributes =
> nit: var name style
Done


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@641
PS8, Line 641: typeAttrs
> nit: style
Done


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h
File src/kudu/common/decimal.h:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@53
PS8, Line 53:static const int32_t MIN_DECIMAL_PRECISION = 0;
> see comment elsewhere about precision 0
Done


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@63
PS8, Line 63:explicit Decimal(int128_t s) :
> I sort of feel like this Decimal instance should retain its precision and s
Yeah, I commented that I was pretty conflicted about this too on one of Dan's 
reviews. Had I created this class with no reference at all I would definitely 
have gone that route.

The only thing that made me leave off precision and scale was the Impala 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 8:

(23 comments)

I think this could do with a couple end-to-end tests from the client writing 
decimals of different precision and making sure they come back reasonably

http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@17
PS8, Line 17: stored
not stored? it must be stored in the metadata somewhere right?


http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@24
PS8, Line 24: int128 suffixes
perhaps we should guard them with a preprocessor check for c++11 instead?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@63
PS8, Line 63: class KuduColumnTypeAttributes {
should this be exported for users? If so I think we need to PIMPL it for ABI 
compatibility (I imagine this is where we might later add stuff like charsets 
for strings?)

If not exported then I think it belongs in schema-internal.h or somesuch


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@177
PS8, Line 177:   /// @todo KUDU-809: make this hard-to-use constructor private.
 :   ///   Clients should use the Builder API. Currently only the 
Python API
 :   ///   uses this old API.
this has been deprecated for several releases now and I dont think python uses 
it anymore. Maybe we can remove it (make it private)?  Changing its signature 
is equally as illegal as removing it.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@323
PS8, Line 323: floating-point
it's not floating point, right?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@328
PS8, Line 328:   /// The precision must be between 0 and 38.
curious what a precision of 0 means... that can only store the value 0?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@343
PS8, Line 343:   /// columns precision.
typo: column's


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@133
PS8, Line 133:   return kudu::DECIMAL128;
nit: weird indentation


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@153
PS8, Line 153: case kudu::DECIMAL32 : return KuduColumnSchema::DECIMAL;
nit: extra space


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@266
PS8, Line 266:   return Status::InvalidArgument("no scale provided for 
decimal column", data_->name);
is a default scale of 0 not reasonable? I seem to recall that sql allows 
DECIMAL(5) and that means 0 through 9


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@285
PS8, Line 285:   }
can we check that no precision/scale are specified for non-decimal column 
types? want to make sure someone doesn't try to do STRING(10) under some 
assumption this would produce a fixed-length or truncated string type (as in 
sql VARCHAR(10))


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@287
PS8, Line 287:   int32_t precision = 0;
 :   if (data_->has_precision) {
 : precision = data_->precision;
 :   }
think ternary would be easier to read here and below


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@297
PS8, Line 297:   KuduColumnTypeAttributes kuduColumnTypeAttributes =
nit: var name style
Also you can just construct this like KuduColumnTypeAttributes attr(precision, 
scale) -- no need for the extra assignment


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@641
PS8, Line 641: typeAttrs
nit: style


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h
File src/kudu/common/decimal.h:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@49
PS8, Line 49:99) * 100) + 99; // 38 9's
well this is fun. have you tested that this works as expected and not getting 
secretly truncated somewhere along the line?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@53
PS8, Line 53:static const int32_t MIN_DECIMAL_PRECISION = 0;
> warning: invalid case style for global constant 'MIN_DECIMAL_PRECISION' [re
see comment elsewhere about precision 0


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@63
PS8, Line 63:explicit Decimal(int128_t s) :
I sort of feel like this Decimal instance should retain its precision and scale 
as a member and then check for compatibility in the appropriate spots. 
Otherwise it will be very easy to get incorrect results if you accidentally 
pass a non-matching decimal Value into something like a 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#8).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored,
but instead are leveraged to map to a correctly
sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because they break the
client by using C++11 features. They may be added back in a
different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal.cc
A src/kudu/common/decimal.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
25 files changed, 665 insertions(+), 140 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/8
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

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 

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#7).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored,
but instead are leveraged to map to a correctly
sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because they break the
client by using C++11 features. They may be added back in a
different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal.cc
A src/kudu/common/decimal.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
25 files changed, 666 insertions(+), 140 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/7
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 6:

(12 comments)

It seems I took a quick look at PS1 some time ago.  Will re-visit this week.

http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/client/schema.h@64
PS5, Line 64: public:
nit: alignment


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@71
PS1, Line 71: : pre
I don't think this is needed unless we want to protect against list-initialized 
constructors.


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@76
PS1, Line 76:   ret
drop


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@81
PS1, Line 81:   ret
drop


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@321
PS1, Line 321:   ///
Add the documented description for the precision parameter.


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@328
PS1, Line 328:   ///
Add the documented description for the scale parameter.


http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto
File src/kudu/common/common.proto:

http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto@54
PS5, Line 54:   DECIMAL32 = 17;
nit: could you add a comment to explain why 15 and 16 are skipped?


http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h
File src/kudu/common/decimal_value.h:

http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h@32
PS5, Line 32:
nit: spacing


http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h@43
PS5, Line 43:
nit: by code guidelines, the indent should be 2 spaces.


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:

http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@50
PS1, Line 50:  DataType type) const {
nit: misaligned line for the second parameter


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@57
PS1, Line 57: true
Why true, not false?  If there is some specific reason, please add a comment 
about that.


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@125
PS1, Line 125:   return strings::Substitute("$0$1 $2",
nit: missed space



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 03 Jan 2018 19:57:40 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 6:

(4 comments)

I only looked at the public API changes.

http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@64
PS6, Line 64: public:
Nit: indentation


http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@70
PS6, Line 70:   explicit KuduColumnTypeAttributes(int32_t precision, int32_t 
scale)
The 'explicit' keyword is only needed to avoid implicit conversions with 
single-arg constructors, no?


http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@75
PS6, Line 75:   const int32_t precision() const {
These accessors are returning an int32_t 'copy', so what value does the 'const' 
keyword add (as in 'const int32_t ...')?


http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@84
PS6, Line 84:  private:
Do you anticipate KuduColumnTypeAttributes evolving for other use cases in the 
future? If so, you should PIMPL the class: define a private Data nested class, 
store precision/scale within it, and have the public class merely store an 
allocated pointer to the nested class.



--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 03 Jan 2018 19:18:24 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#6).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored,
but instead are leveraged to map to a correctly
sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because they break the
client by using C++11 features. They may be added back in a
different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal_value.cc
A src/kudu/common/decimal_value.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
25 files changed, 630 insertions(+), 140 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/6
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#5).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored,
but instead are leveraged to map to a correctly
sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because they break the
client by using C++11 features. They may be added back in a
different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal_value.cc
A src/kudu/common/decimal_value.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
25 files changed, 629 insertions(+), 139 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/5
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#4).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored,
but instead are leveraged to map to a correctly
sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because they break the
client by using C++11 features. They may be added back in a
different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal_value.cc
A src/kudu/common/decimal_value.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
25 files changed, 629 insertions(+), 139 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/4
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#3).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored,
but instead are leveraged to map to a correctly
sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because they break the
client by using C++11 features. They may be added back in a
different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal_value.cc
A src/kudu/common/decimal_value.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
25 files changed, 636 insertions(+), 139 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/3
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8830

to look at the new patch set (#2).

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Note: I have 2 failing tests but want to push
this to start reviews while I work on them.

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored,
but instead are leveraged to map to a correctly
sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal_value.cc
A src/kudu/common/decimal_value.h
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/int128.h
24 files changed, 623 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/2
--
To view, visit http://gerrit.cloudera.org:8080/8830
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot