You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2019/07/16 05:13:26 UTC
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13869
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/all_types-itest.cc
8 files changed, 261 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/1
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#17).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/char-itest.cc
10 files changed, 370 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/17
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#29).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 321 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/29
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 29
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#3).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/all_types-itest.cc
8 files changed, 226 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/3
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#20).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 309 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/20
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 20
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 5:
(1 comment)
Looks good but I'm still hoping you can add that end-to-end test I suggested in PS3.
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/predicate-test.cc@1200
PS3, Line 1200: TEST_F(PredicateTest, TestCharPredicates) {
> I'm not sure it would work here as most of this is the same, but there are
It'll reduce a fair amount of test code so I think it's worth it.
Looking at the code, I don't think you want a typed test as much as a value parameterized test, since the parameterization is across enum values (rather than distinct classes or types). See https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#value-parameterized-tests for details, and/or grep for TEST_P in the codebase.
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 19 Jul 2019 16:22:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 17:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/client/schema.cc@346
PS16, Line 346: ->precision.value()) {
> nit here and below in case of not applicable attributes:
Done
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/client/schema.cc@408
PS16, Line 408: s::InvalidArgum
> nit: it seems the two lines above also followed this pattern, but why paren
Done
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/integration-tests/char-itest.cc
File src/kudu/integration-tests/char-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/integration-tests/char-itest.cc@90
PS16, Line 90: ASSERT_OK(read.GetVarchar("value", &value));
> Do we have a coverage for the cases when CHAR column values are read via VA
That wouldn't work due to this: https://github.com/apache/kudu/blob/de49f8e/src/kudu/client/scan_batch.cc#L219-L225
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 30 Aug 2019 14:26:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#19).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 309 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/19
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 19
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 21:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h@99
PS21, Line 99: KuduColumnTypeAttributes(int8_t precision, int8_t scale, uint16_t length);
Why do we want this constructor? Isn't it an error to specify precision, scale, and length?
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/integration-tests/varchar-itest.cc
File src/kudu/integration-tests/varchar-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/integration-tests/varchar-itest.cc@68
PS21, Line 68: char/varchar
Just "varchar" now.
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 21
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 19:04:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 18:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13869/17/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/17/src/kudu/client/schema.h@89
PS17, Line 89: explicit KuduColumnTypeAttributes(uint16_t length);
> Thank you for adding this new constructor. Adding 'explicit' as recommende
Done
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/integration-tests/char-itest.cc
File src/kudu/integration-tests/char-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/integration-tests/char-itest.cc@90
PS16, Line 90: ASSERT_OK(read.GetVarchar("value", &value));
> All right, it seems we assume there are no coercion for any pair of types,
Done
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 30 Aug 2019 17:34:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 3:
(5 comments)
Would be nice to see an end-to-end test that proves the expected padding/truncation semantics for the new types.
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/predicate-test.cc@1200
PS3, Line 1200: TEST_F(PredicateTest, TestCharPredicates) {
Would it be possible to type-parameterize these new tests, perhaps along with TestStringPredicates and TestBinaryPredicates?
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
PS3:
Do we want to doc that the char getter does _not_ pad with whitespace?
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.h@84
PS3, Line 84: /// The length of a CHAR or VARCHAR column.
"Maximum length". Should also doc whether we're talking about bytes or characters.
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.h@398
PS3, Line 398: fixed length of a CHAR column
In light of the fact that we truncate excess whitespace when writing and don't pad when reading, what does "fixed length" mean?
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.cc@320
PS3, Line 320: if (data_->type.value() == KuduColumnSchema::DECIMAL) {
Maybe convert this into a switch on data_->type.value()?
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Jul 2019 17:21:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 27: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/27/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13869/27/src/kudu/client/schema.cc@276
PS27, Line 276: data_->length = length;
nit: what happens if setting length to 0? Does it make sense to add a test scenario to cover this case?
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 27
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 02 Oct 2019 17:12:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 11:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc@1145
PS11, Line 1145: TestStringPredicates
Nit: maybe rename this to something more generic now that it's not just string predicates? Like, these are all predicates whose values are stored in indirect data, right?
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc@1165
PS11, Line 1165: default:
Nit: I would actually reserve the default case for a LOG(FATAL) or some such, since we wouldn't want anyone to extend the list in ::testing::Values() without also adding a new case here.
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc
File src/kudu/integration-tests/char-itest.cc:
PS11:
This is fine, though extending client-test would have been fine too (fewer LOC I suspect).
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc@68
PS11, Line 68: // Insert Row
May want to explain what's special about the row we're inserting (i.e. what we're trying to test).
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc@85
PS11, Line 85: CHECK_OK
ASSERT_OK
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 22 Jul 2019 19:09:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#8).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/char-itest.cc
10 files changed, 338 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/8
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 29: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 29
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 22 Oct 2019 16:19:53 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Reviewed-on: http://gerrit.cloudera.org:8080/13869
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 321 insertions(+), 73 deletions(-)
Approvals:
Grant Henke: Looks good to me, approved
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 30
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#22).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 310 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/22
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 22
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#13).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/char-itest.cc
10 files changed, 348 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/13
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 25:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h@99
PS21, Line 99: ///@{
> yep, somehow I messed this up, fixed
turns out we need this after all, although it can be private, but KuduColumnStorageAttributes needed to make some new friends
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 25
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 20:57:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#10).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/char-itest.cc
10 files changed, 338 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/10
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 17:
(2 comments)
LGTM, just please address the suggestion from TidyBot.
http://gerrit.cloudera.org:8080/#/c/13869/17/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/17/src/kudu/client/schema.h@89
PS17, Line 89: KuduColumnTypeAttributes(uint16_t length);
> warning: single-argument constructors must be marked explicit to avoid unin
Thank you for adding this new constructor. Adding 'explicit' as recommended by tidy bot will help to avoid surprises: please address TidyBot's suggestion.
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/integration-tests/char-itest.cc
File src/kudu/integration-tests/char-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/integration-tests/char-itest.cc@90
PS16, Line 90: ASSERT_OK(read.GetVarchar("value", &value));
> That wouldn't work due to this: https://github.com/apache/kudu/blob/de49f8e
All right, it seems we assume there are no coercion for any pair of types, so CHAR to/from VARCHAR should not work the same as any other pair of types.
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 30 Aug 2019 16:43:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 28: Code-Review+2
LGTM just need to fix the IWYU error
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 28
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 22 Oct 2019 13:25:26 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#24).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 311 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/24
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 24
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 29: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 29
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 22 Oct 2019 16:58:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 25: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 25
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 21:05:15 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#27).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 309 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/27
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 27
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/predicate-test.cc@1200
PS3, Line 1200: TEST_F(PredicateTest, TestCharPredicates) {
> Would it be possible to type-parameterize these new tests, perhaps along wi
I'm not sure it would work here as most of this is the same, but there are differences based on types (AddColumn()->Type(), SetChar()/SetVarchar()/etc). I'm not too familiar with gtest, so I'm not sure if it's possible to do this with it, and if it is, is it worth it? In the very least (if I can get the type somehow), it would require two switches.
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
PS3:
> Do we want to doc that the char getter does _not_ pad with whitespace?
renamed these too to GetUnpaddedChar based on Grant's suggestion on the parent commit in partial row, it should make it clear.
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.h@84
PS3, Line 84: /// The maximum length of a CHAR or VARCHAR column in characters.
> "Maximum length". Should also doc whether we're talking about bytes or char
Done
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.h@398
PS3, Line 398: maximum length of a CHAR or a
> In light of the fact that we truncate excess whitespace when writing and do
nothing, didn't notice it wasn't changed here
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/schema.cc@320
PS3, Line 320: switch (data_->type.value()) {
> Maybe convert this into a switch on data_->type.value()?
Done
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 19 Jul 2019 13:02:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 13:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc@1145
PS11, Line 1145:
> Nit: maybe rename this to something more generic now that it's not just str
Done
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc@1165
PS11, Line 1165: case KuduColumnSchema::VARCHAR:
> Nit: I would actually reserve the default case for a LOG(FATAL) or some suc
that's why I added a DCHECK() but LOG(FATAL) is a better approach, fixed.
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc
File src/kudu/integration-tests/char-itest.cc:
PS11:
> This is fine, though extending client-test would have been fine too (fewer
Should I move it there?
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc@68
PS11, Line 68: shared_ptr<KuduSession> session = client_->NewSession();
> May want to explain what's special about the row we're inserting (i.e. what
Done
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc@85
PS11, Line 85: for (con
> ASSERT_OK
Done
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 14:05:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/13/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/13/src/kudu/client/predicate-test.cc@1169
PS13, Line 1169: LOG(FATAL) << "Invalid type";
> Well, I guess it's not super useful in this context.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Aug 2019 20:41:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#23).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 301 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/23
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 23
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 16:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/client/schema.h@85
PS16, Line 85: KuduColumnTypeAttributes(int8_t precision, int8_t scale, uint16_t length);
Is this update with prior versions? I remember we strive to keep API/ABI compatibility, so older code would be compile with new versions of Kudu client library.
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/client/schema.cc@346
PS16, Line 346: is not valid on a $0 column
nit here and below in case of not applicable attributes:
'is not valid on a $0 column' --> 'is not applicable for column $0'
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/client/schema.cc@408
PS16, Line 408: (data_->length)
nit: it seems the two lines above also followed this pattern, but why parentheses are necessary? Maybe, drop them (and do that for those two lines above as well)?
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/integration-tests/char-itest.cc
File src/kudu/integration-tests/char-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13869/16/src/kudu/integration-tests/char-itest.cc@90
PS16, Line 90: ASSERT_OK(read.GetVarchar("value", &value));
Do we have a coverage for the cases when CHAR column values are read via VARCHAR-related accessors and vice versa? If not, does it make sense to add such?
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 28 Aug 2019 18:35:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#7).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/all_types-itest.cc
8 files changed, 248 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/7
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 22:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h@99
PS21, Line 99: KuduColumnTypeAttributes(int8_t precision, int8_t scale, uint16_t length);
> Why do we want this constructor? Isn't it an error to specify precision, sc
you're right, removed
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/integration-tests/varchar-itest.cc
File src/kudu/integration-tests/varchar-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/integration-tests/varchar-itest.cc@68
PS21, Line 68: varchar valu
> Just "varchar" now.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 22
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 19:43:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#11).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/char-itest.cc
10 files changed, 346 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/11
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 14: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 06:25:11 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 18: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 03 Sep 2019 21:26:57 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 27:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/27/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13869/27/src/kudu/client/schema.cc@276
PS27, Line 276: data_->length = length;
> It will result in an InvalidArgument as per https://gerrit.cloudera.org/c/1
If we don't have it covered by a test, I would add one: that would not hurt, I think. The idea is to:
* make sure that the error condition is handled
* catch a regression, if any, when the related code changes
* be explicit where we expect an error to surface
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 27
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 15:01:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 22:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h@99
PS21, Line 99: KuduColumnTypeAttributes(int8_t precision, int8_t scale, uint16_t length);
> you're right, removed
It's still in as of PS22?
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 22
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 20:17:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#25).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 307 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/25
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 25
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 27: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 27
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 02 Oct 2019 14:09:56 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#21).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 312 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/21
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 21
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#28).
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
KUDU-1938 Add C++ client support for VARCHAR pt 2
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/varchar-itest.cc
10 files changed, 320 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/28
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 28
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#9).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/char-itest.cc
10 files changed, 338 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/9
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13869/13/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/13/src/kudu/client/predicate-test.cc@1169
PS13, Line 1169: LOG(FATAL) << "Invalid type";
Log the value of GetParam() too.
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc
File src/kudu/integration-tests/char-itest.cc:
PS11:
> Should I move it there?
If you want. I don't have a strong opinion.
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 20:39:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 28:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/27/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13869/27/src/kudu/client/schema.cc@276
PS27, Line 276: data_->length = length;
> If we don't have it covered by a test, I would add one: that would not hurt
Done
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 28
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 22 Oct 2019 10:41:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 23:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/13869/21/src/kudu/client/schema.h@99
PS21, Line 99: KuduColumnTypeAttributes& operator=(const KuduColumnTypeAttributes& other);
> It's still in as of PS22?
yep, somehow I messed this up, fixed
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 23
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 20:32:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 13: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/13/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/13/src/kudu/client/predicate-test.cc@1169
PS13, Line 1169: LOG(FATAL) << "Invalid type";
> it's a C-style enum so we'll only see an integer, should I still do it?
Well, I guess it's not super useful in this context.
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 22:37:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add C++ client support for VARCHAR pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add C++ client support for VARCHAR pt 2
......................................................................
Patch Set 27:
(1 comment)
> Patch Set 27: Code-Review+2
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/13869/27/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13869/27/src/kudu/client/schema.cc@276
PS27, Line 276: data_->length = length;
> nit: what happens if setting length to 0? Does it make sense to add a test
It will result in an InvalidArgument as per https://gerrit.cloudera.org/c/13869/27/src/kudu/client/schema.cc#329
Not sure about the test, should I add one?
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 27
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 04 Oct 2019 12:31:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#18).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/char-itest.cc
10 files changed, 370 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/18
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#12).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/char-itest.cc
10 files changed, 348 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/12
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 8:
(1 comment)
> Looks good but I'm still hoping you can add that end-to-end test I suggested in PS3.
Sorry, missed this comment. Added char-itest.cc to integration-test, is that what you had in mind?
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/3/src/kudu/client/predicate-test.cc@1200
PS3, Line 1200:
> It'll reduce a fair amount of test code so I think it's worth it.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 20 Jul 2019 12:42:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13869
to look at the new patch set (#4).
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adds support for CHAR and VARCHAR types to the C++ client.
Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
---
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/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/integration-tests/all_types-itest.cc
8 files changed, 281 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/13869/4
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 )
Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13869/13/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13869/13/src/kudu/client/predicate-test.cc@1169
PS13, Line 1169: LOG(FATAL) << "Invalid type";
> Log the value of GetParam() too.
it's a C-style enum so we'll only see an integer, should I still do it?
http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc
File src/kudu/integration-tests/char-itest.cc:
PS11:
> If you want. I don't have a strong opinion.
I guess I'll leave it here unless someone has a strong opinion to move it. I kinda find it clearer here in an itest, client-test is 6k+ LOC anyways.
--
To view, visit http://gerrit.cloudera.org:8080/13869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 21:16:06 +0000
Gerrit-HasComments: Yes