[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Additionally this patch adds user defined numberic literals
_i128 and _u128 to allow for literals and constants to be used
until official compiler support is added.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Reviewed-on: http://gerrit.cloudera.org:8080/8533
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 423 insertions(+), 0 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Dan Burkert: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 18
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Dan Burkert (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 17: Code-Review+2

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8533/17/src/kudu/util/int128.h@57
PS17, Line 57: VALUE * 10 <= UINT128_MAX - CharValue(C)
> Good question.
Yeah, I was mostly worried about legibility.  SGTM to keep it the way it is.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 17
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 11 Dec 2017 20:09:20 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 17:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8533/17/src/kudu/util/int128.h@57
PS17, Line 57: VALUE * 10 <= UINT128_MAX - CharValue(C)
> could these first two clauses be simplified to one as
Good question.

The existing checks are saying:
1. Can I shift the existing value to the left without overflow.
2. When adding the new character am I still less than UINT128_MAX.

I worked a sample example of your idea to be sure, but that's not equivalent. 
For example:

Valid:
LET UINT128_MAX=105
Let VALUE=10
Let C=5
10 <= 1 = (105 - 5) / 10

Invalid:
LET UINT128_MAX=105
Let VALUE=10
Let C=6
10 <= 9.9 = (105 - 6) / 10

I think there is a way I could reduce the check to one statement leveraging the 
fact that I know a value would overflow, but I don't think it would read more 
clearly. Given this is done at compile time, I don't think we should worry to 
much about the performance difference.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 17
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 11 Dec 2017 18:07:27 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Dan Burkert (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 17:

(1 comment)

Nice, these literals are way-cool.

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

http://gerrit.cloudera.org:8080/#/c/8533/17/src/kudu/util/int128.h@57
PS17, Line 57: VALUE * 10 <= UINT128_MAX - CharValue(C)
could these first two clauses be simplified to one as

(VALUE <= (UINT128_MAX - CharValue(C)) / 10)

I know there are subtle overflow/rounding things going on here, so not entirely 
sure.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 17
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 00:27:32 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Additionally this patch adds user defined numberic literals
_i128 and _u128 to allow for literals and constants to be used
until official compiler support is added.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 423 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 17
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Additionally this patch adds user defined numberic literals
_i128 and _u128 to allow for literals and constants to be used
until official compiler support is added.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 423 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 16
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Dan Burkert (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 15:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8533/15/src/kudu/util/int128.h@43
PS15, Line 43:   std::ostream& operator<<(std::ostream& os, const __int128& 
val);
> I only defined int128_t within the Kudu namespace to avoid any issues.
ah ok, SGTM.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 30 Nov 2017 18:43:18 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 15:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8533/15/src/kudu/util/int128.h@43
PS15, Line 43:   std::ostream& operator<<(std::ostream& os, const __int128& 
val);
> You might consider defining this with int128_t instead of __int128, but fee
I only defined int128_t within the Kudu namespace to avoid any issues.

Everywhere else I used __int128. If I use int128_t here it causes issues in 
some of the gutil code.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 30 Nov 2017 18:41:05 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Dan Burkert (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8533/15/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/15/src/kudu/util/int128-test.cc@20
PS15, Line 20: #include 
use  to be consistent with the  already here.  Pretty sure 
IWYU allows either.


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

http://gerrit.cloudera.org:8080/#/c/8533/15/src/kudu/util/int128.h@43
PS15, Line 43:   std::ostream& operator<<(std::ostream& os, const __int128& 
val);
You might consider defining this with int128_t instead of __int128, but feel 
free to ignore if there's a good reason not to.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 30 Nov 2017 18:37:40 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 325 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 325 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 321 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 13
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 11:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers-test.cc
File src/kudu/gutil/strings/numbers-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers-test.cc@29
PS10, Line 29: std::string
> nit: you could use
Done


http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers-test.cc@30
PS10, Line 30:   ASSERT_EQ("170141183460469231731687303715884105727", maxStr);
> nit: expected value comes first.
Done


http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers.cc
File src/kudu/gutil/strings/numbers.cc:

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers.cc@1077
PS10, Line 1077:   static const unsigned __int128 TWENTY_DIGITS =
> static const ... ?
Done


http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/util/int128-test.cc@54
PS10, Line 54:   ASSERT_
> nit: it's not important, but if the expected value comes first, then it's e
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 16 Nov 2017 05:39:28 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 321 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Alexey Serbin (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers-test.cc
File src/kudu/gutil/strings/numbers-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers-test.cc@29
PS10, Line 29: std::string
nit: you could use

using std::string;

and drop the std:: prefix.


http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers-test.cc@30
PS10, Line 30:   ASSERT_EQ(maxStr, "170141183460469231731687303715884105727");
nit: expected value comes first.


http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers.cc
File src/kudu/gutil/strings/numbers.cc:

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/gutil/strings/numbers.cc@1077
PS10, Line 1077:   uint64 u = static_cast(i);
static const ... ?


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@39
PS7, Line 39: {0, 1, 1234567890};
> I broke it into 2 separate tests. 1 to test printing UINT128_MAX and a diff
sgtm


http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/util/int128-test.cc@54
PS10, Line 54:
nit: it's not important, but if the expected value comes first, then it's 
easier to parse the error message if the assertion triggers.


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

http://gerrit.cloudera.org:8080/#/c/8533/10/src/kudu/util/int128.h@28
PS10, Line 28:(__SIZEOF_INT128__ * __CHAR_BIT__ - 1)) - 1))
great, now it looks more comprehensive



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 16 Nov 2017 05:24:07 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@39
PS7, Line 39: {0, 1, 1234567890};
> oh, right.  Sorry for the mess.  Indeed, I was interested to see that INT12
I broke it into 2 separate tests. 1 to test printing UINT128_MAX and a 
different one to test the math.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 16 Nov 2017 05:06:31 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Alexey Serbin (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@39
PS7, Line 39: {0, 1, 1234567890};
> Good test recommendations just printing UINT128_MAX showed I was missing a
oh, right.  Sorry for the mess.  Indeed, I was interested to see that 
INT128_MAX * 2 + 1 would print value of 2^128 - 1



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 16 Nov 2017 05:04:58 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@39
PS7, Line 39: {0, 1, 1234567890};
> Is that an error or it's just a warning?
Good test recommendations just printing UINT128_MAX showed I was missing a 0 in 
one of my constants.

I think a test that verifies  (INT128_MAX * 2) + 1 == UINT128_MAX is what you 
are looking for. Added that. Let me know if I misunderstood though. I added a 
-1 cast test too.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 16 Nov 2017 04:59:53 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 315 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Alexey Serbin (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@39
PS7, Line 39: {0, 1, 1234567890};
> Is that an error or it's just a warning?
If you use static_cast(INT128_MAX - 1) * static_cast(2), 
does it compile OK?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 15 Nov 2017 23:27:05 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Alexey Serbin (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@39
PS7, Line 39: {0, 1, 1234567890};
> INT128_MAX * 2 will result in a compilation error with "warning: overflow i
Is that an error or it's just a warning?

Anyway, I guess I meant ((INT128_MAX - 1) * 2 - 1) or maybe just (INT128_MAX - 
1) * 2.

What about static_cast(-1)?  Does it produce the assumed 2^128 - 1?  
Or it should be something else?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 15 Nov 2017 23:23:11 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 299 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 8:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@29
PS7, Line 29: ST(TestInt128, TestOstreamSigned) {
> maybe, it's also worth to verify how INT128_MIN is printed out.
Done


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@31
PS7, Line 31:  {"0", "-1", "1", "-1234567890", "-170
> nit: consider using 'arraysize' macro here
Done


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@32
PS7, Line 32: ze_t i = 0;
> nit: ostringstream
Done


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@34
PS7, Line 34: EGERS[i];
> nit: expected value comes first
Done


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@39
PS7, Line 39: eamUnsigned) {
> mind adding INT128_MAX * 2 into the list (sure, it should be properly caste
INT128_MAX * 2 will result in a compilation error with "warning: overflow in 
expression;" .



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 15 Nov 2017 22:00:27 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 299 insertions(+), 0 deletions(-)


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

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

[kudu-CR] Add initial internal INT128/ int128 support

[Alexey Serbin (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 7:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc
File src/kudu/util/int128-test.cc:

http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@29
PS7, Line 29: int128_t INTEGERS[] = {0, -1, 1, -1234567890};
maybe, it's also worth to verify how INT128_MIN is printed out.


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@31
PS7, Line 31: sizeof(INTEGERS) / sizeof(INTEGERS[0])
nit: consider using 'arraysize' macro here


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@32
PS7, Line 32: stringstream
nit: ostringstream


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@34
PS7, Line 34: (ss.str(),
nit: expected value comes first


http://gerrit.cloudera.org:8080/#/c/8533/7/src/kudu/util/int128-test.cc@39
PS7, Line 39: {0, 1, 1234567890};
mind adding INT128_MAX * 2 into the list (sure, it should be properly casted, 
etc.)?

Also, it's interesting what static_cast(-1) would look like when 
it's printed out.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 15 Nov 2017 19:12:09 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[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/8533

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 298 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[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/8533

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 298 insertions(+), 0 deletions(-)


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

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

[kudu-CR] Add initial internal INT128/ int128 support

[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/8533

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 297 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8533/3/src/kudu/util/int128.h@21
PS3, Line 21: #ifndef KUDU_UTIL_INT128_H
> Prefer '#pragma once' for new headers.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 14 Nov 2017 22:01:06 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[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/8533

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 292 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Dan Burkert (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8533/3/src/kudu/util/int128.h@21
PS3, Line 21: #ifndef KUDU_UTIL_INT128_H
Prefer '#pragma once' for new headers.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 14 Nov 2017 19:22:58 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc
File src/kudu/util/int128.cc:

http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@25
PS2, Line 25: // Print the value in base 10 by converting v into parts that are 
base
> Right, will fix. This was a result of modifying similar Impala functionalit
Done


http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@30
PS2, Line 30: v = -v;
> This is wrong for the minimum value. FastInt128ToBufferLeft does it right.
Done


http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@37
PS2, Line 37: std::ostream& operator<<(std::ostream& os, const unsigned 
__int128& val) {
> I think it'd be better to call into that implementation here in order to de
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 14 Nov 2017 19:15:54 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[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/8533

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 294 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add initial internal INT128/ int128 support

[Dan Burkert (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc
File src/kudu/util/int128.cc:

http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@30
PS2, Line 30: v = -v;
This is wrong for the minimum value. FastInt128ToBufferLeft does it right.


http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@37
PS2, Line 37: std::ostream& operator<<(std::ostream& os, const unsigned 
__int128& val) {
> The main reason is that this implementation is very heavily borrowed from i
I think it'd be better to call into that implementation here in order to define 
operator<<, so we don't have two implementations of the same algorithm in the 
codebase.  The google impl is also a bit simpler (no loop).



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 13 Nov 2017 22:36:29 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc
File src/kudu/util/int128.cc:

http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@25
PS2, Line 25: // Print the value in base 10 by converting v into parts that are 
base
> I think this docstring is referring to the unsigned version.
Right, will fix. This was a result of modifying similar Impala functionality.


http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@37
PS2, Line 37: std::ostream& operator<<(std::ostream& os, const unsigned 
__int128& val) {
> Why isn't this implemented using FastInt128ToBufferLeft and a stack-allocat
The main reason is that this implementation is very heavily borrowed from 
impala here: 
https://github.com/apache/incubator-impala/blob/2e63752858d71cc745534367a686980e060a8180/be/src/runtime/multi-precision.cc



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 13 Nov 2017 22:27:39 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Dan Burkert (Code Review)]

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

Change subject: Add initial internal INT128/__int128 support
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc
File src/kudu/util/int128.cc:

http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@25
PS2, Line 25: // Print the value in base 10 by converting v into parts that are 
base
I think this docstring is referring to the unsigned version.


http://gerrit.cloudera.org:8080/#/c/8533/2/src/kudu/util/int128.cc@37
PS2, Line 37: std::ostream& operator<<(std::ostream& os, const unsigned 
__int128& val) {
Why isn't this implemented using FastInt128ToBufferLeft and a stack-allocated 
buffer?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 13 Nov 2017 22:16:17 +
Gerrit-HasComments: Yes

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

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

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

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

Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 327 insertions(+), 0 deletions(-)


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

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

[kudu-CR] Add initial internal INT128/ int128 support

[Grant Henke (Code Review)]

Grant Henke has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8533


Change subject: Add initial internal INT128/__int128 support
..

Add initial internal INT128/__int128 support

This patch adds internal support for INT128/__int128 in Kudu.
This preliminary functionality is required to support the DECIMAL
type in KUDU-721.

Follow up patches will be added to review and discus making the
INT128 column type publicly exposed.

Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
---
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/common/common.proto
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/integral_types.h
M src/kudu/gutil/mathlimits.cc
M src/kudu/gutil/mathlimits.h
A src/kudu/gutil/strings/numbers-test.cc
M src/kudu/gutil/strings/numbers.cc
M src/kudu/gutil/strings/numbers.h
M src/kudu/gutil/strings/substitute.h
M src/kudu/gutil/type_traits.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/int128-test.cc
A src/kudu/util/int128.cc
A src/kudu/util/int128.h
21 files changed, 327 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I36458a54dfdd28be03f80d83688c0d658944e8e1
Gerrit-Change-Number: 8533
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke