You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/02/02 18:05:29 UTC
[kudu-CR] [tests] Add more test coverage for INT128 columns
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9193
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
[tests] Add more test coverage for INT128 columns
Improves test coverage of reading and writing INT128
columns.
Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
5 files changed, 40 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/9193/1
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@gmail.com>
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9193 )
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 05 Feb 2018 17:50:14 +0000
Gerrit-HasComments: No
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9193 )
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
Patch Set 8:
(2 comments)
LGTM, but seems like it may be a good opportunity to fill in the 64-bit tests as well.
http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:
http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/cfile-test.cc@563
PS8, Line 563: // Test for BitShuffle builder for UINT8, INT8, UINT16, INT16, UINT32, INT32, FLOAT, DOUBLE
Update this comment. Also, is the omission of the 64 bit types intentional here? Seems like maybe we overlooked it at some point.
http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/encoding-test.cc@694
PS8, Line 694: // Test for bitshuffle block, for INT32, INT128, FLOAT, DOUBLE
Same here, seems like 64bit should be added as well.
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 05 Feb 2018 16:08:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9193
to look at the new patch set (#3).
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
[tests] Add more test coverage for INT128 columns
Improves test coverage of reading and writing INT128
columns.
Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
5 files changed, 46 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/9193/3
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9193 )
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:
http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/cfile/cfile-test.cc@553
PS2, Line 553:
> no need for this space between closing brackets anymore; this was fixed in
Done
http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/tablet/all_types-scan-correctness-test.cc
File src/kudu/tablet/all_types-scan-correctness-test.cc:
http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/tablet/all_types-scan-correctness-test.cc@585
PS2, Line 585: // TODO: Uncomment when adding 128 bit support to RLE
> Probably worth having a tracking ticket for this since I imagine we won't d
Agreed. Created KUDU-2284 and updated all places where this was used.
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Feb 2018 18:41:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9193 )
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:
http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/cfile-test.cc@563
PS8, Line 563: // Test for BitShuffle builder for UINT8, INT8, UINT16, INT16, UINT32, INT32,
> Update this comment. Also, is the omission of the 64 bit types intentional
Done
http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/encoding-test.cc@694
PS8, Line 694: // Test for bitshuffle block, for INT32, INT64, INT128, FLOAT, DOUBLE
> Same here, seems like 64bit should be added as well.
Done
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 05 Feb 2018 16:20:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9193
to look at the new patch set (#2).
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
[tests] Add more test coverage for INT128 columns
Improves test coverage of reading and writing INT128
columns.
Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
5 files changed, 41 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/9193/2
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9193
to look at the new patch set (#5).
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
[tests] Add more test coverage for INT128 columns
Improves test coverage of reading and writing INT128
columns.
Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
6 files changed, 49 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/9193/5
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9193
to look at the new patch set (#4).
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
[tests] Add more test coverage for INT128 columns
Improves test coverage of reading and writing INT128
columns.
Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
6 files changed, 48 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/9193/4
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9193 )
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
[tests] Add more test coverage for INT128 columns
Improves test coverage of reading and writing INT128
columns and fixes a predicate issue exposed by those
tests.
Also adds INT64 tests where it was missing.
Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Reviewed-on: http://gerrit.cloudera.org:8080/9193
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@cloudera.com>
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
6 files changed, 69 insertions(+), 11 deletions(-)
Approvals:
Kudu Jenkins: Verified
Dan Burkert: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9193
to look at the new patch set (#9).
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
[tests] Add more test coverage for INT128 columns
Improves test coverage of reading and writing INT128
columns and fixes a predicate issue exposed by those
tests.
Also adds INT64 tests where it was missing.
Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
6 files changed, 69 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/9193/9
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tests] Add more test coverage for INT128 columns
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9193 )
Change subject: [tests] Add more test coverage for INT128 columns
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:
http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/cfile/cfile-test.cc@553
PS2, Line 553:
no need for this space between closing brackets anymore; this was fixed in C++11.
http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/tablet/all_types-scan-correctness-test.cc
File src/kudu/tablet/all_types-scan-correctness-test.cc:
http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/tablet/all_types-scan-correctness-test.cc@585
PS2, Line 585: // TODO: Uncomment when adding 128 bit support to RLE
> warning: missing username/bug in TODO [google-readability-todo]
Probably worth having a tracking ticket for this since I imagine we won't do it right away.
--
To view, visit http://gerrit.cloudera.org:8080/9193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Feb 2018 18:28:14 +0000
Gerrit-HasComments: Yes