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/06/28 13:12:34 UTC
[kudu-CR] [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13760
Change subject: [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
[WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
A src/kudu/util/char_util.h
14 files changed, 306 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/1
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#14).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
21 files changed, 557 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/14
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 33:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/33/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/33/src/kudu/util/char_util.cc@29
PS33, Line 29: num_utf8_chars += (*str++ & 0xc0) != 0x80;
This fix is rather concerning given how late in the code review cycle it manifested. Looks like we don't have any explicit test coverage for UTF8Truncate? Could you add some?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 33
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:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#7).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client. Follow
up commits will add integration to other clients. The CHAR and VARCHAR types are
parameterized with a length column type attribute similar to DECIMAL's scale and
precision. Internally both of them are stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/util/char_util.h
19 files changed, 477 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/7
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#23).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
BINARY.
The maximum length for VARCHAR is 65,535 and 255 for CHAR. Both of them
are truncated to the length of the column and trailing spaces left after
truncation to length are also removed from CHAR fields.
Truncation happens before persisting the data on the server side to
prevent wasting space and also for the predicates to be applied correctly.
The maximum lengths were chosen for compatibility reasons. Apache Impala
has a maximum length of 255 characters for CHAR and 65,353 for VARCHAR,
major RDBMSs I checked also had similar, or lower limits.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 326 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/23
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#12).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
23 files changed, 540 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/12
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 28:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row-test.cc@267
PS26, Line 267: EXPECT_OK(row.Unset(4));
> By 'Done' you meant these scenarios are in other changelist, like https://g
I messed up the commit, I started to write the patch on the top of pt 4, then stashed it and edited part 1 but apparently I forgot to pop this change off the stash. Added it now
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Fri, 30 Aug 2019 14:25:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 10:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/scan_batch.h@266
PS10, Line 266: Status GetCharVarchar(const Slice& col_name, Slice* val) const WARN_UNUSED_RESULT;
Similar to GetBinary and GetString, I think these should be different methods, GetChar and GetVarChar.
The semantics and expectations between the types are different enough to warrant different setters and getters.
See my comments elsewhere about padding he CHAR type client side. Separating these methods will make this easy.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/schema.cc@348
PS10, Line 348: } else if (data_->type.value() == KuduColumnSchema::CHAR ||
I think the standard max CHAR length is 254.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/schema.cc@377
PS10, Line 377: uint16_t length = (data_->length) ? data_->length.value() : kDefaultCharLength;
This should likely default to 0 given it's "unused" in this case. That matches how precision is handled.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc@1124
PS10, Line 1124: ColumnSchema chars("chars", CHAR);
nit: "char"
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.h@167
PS10, Line 167: Status SetCharVarchar(const Slice& col_name, const Slice& val) WARN_UNUSED_RESULT;
Similar to SetBinary and SetString, I think these should be different methods, SetChar and SetVarChar.
The semantics and expectations between the types are different enough to warrant different setters and getters.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.cc@455
PS10, Line 455: if (type == CHAR) {
I think we should follow the same semantics as Impala: https://impala.apache.org/docs/build/html/topics/impala_char.html
If you store a CHAR value containing trailing spaces in a table, those trailing spaces are not stored in the data file.
Essentially when setting a CHAR value always truncate trailing spaces, and when getting the CHAR value always pad with the required spaces. This saves space on the wire and disk. This also ensures predicates behave correctly.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h@634
PS10, Line 634: struct DataTypeTraits<CHAR> : public DerivedTypeTraits<BINARY>{
Can this be DerivedTypeTraits STRING to unify all the UTF8 and debug string stuff?
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h@647
PS10, Line 647: struct DataTypeTraits<VARCHAR> : public DerivedTypeTraits<BINARY>{
Can this be DerivedTypeTraits STRING to unify all the UTF8 and debug string stuff?
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/util/char_util.h@28
PS10, Line 28: static const uint16_t kMaxCharLength = pow(2, 16) - 1; // 65535
I think the standard for max CHAR length is 255 and max VARCHAR is 65535.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/util/char_util.h@29
PS10, Line 29: static const uint16_t kDefaultCharLength = kMaxCharLength;
Do we need to have a default? Shouldn't we require it to be specified fo CHAR and VARCHAR columns?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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: Sat, 29 Jun 2019 12:30:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
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/13760
to look at the new patch set (#32).
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
KUDU-1938 Add support for VARCHAR pt 1
Introduces the VARCHAR data type to the server. Follow up commits will
add integration to the clients. The VARCHAR type is parameterized with a
length column type attribute similar to DECIMAL's scale and precision.
Internally it's stored as BINARY.
The maximum length for VARCHAR is 65,535 characters. If a value longer
than the "n" is submitted for a VARCHAR(n) column the value is truncated
to "n" characters before persisting the data on the server side.
The maximum length was chosen for compatibility reasons. Apache Impala
has a maximum length of 65,535 *bytes* for VARCHAR and major RDBMSs I
checked also have a similar limits either in characters or bytes, mostly
configurable.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 255 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/32
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 32
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 support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 19:
(3 comments)
I had some questions about how CHAR padding should be handled, so Attila, Grant and I had a quick chat. I'm summarizing our conclusions below:
1. It makes sense for the CHAR that's stored on-disk to be truncated down to its max length and be free of trailing whitespace. This patch currently only handles the former; it needs to also handle the latter.
2. For simplicity's sake, let's only offer whatever client-side CHAR APIs we actually need. For DECIMAL, Impala bypasses the "safe" handling in KuduScanBatch and the Python bindings do decimal scaling themselves. If CHAR is to be comparable, we should only offer unpadded APIs; it's a simple matter for query engines (and other applications) to pad the CHAR themselves. This should simplify the C++ client API in that there's no longer any need for KuduScanBatch's getter to copy the CHAR into a new std::string; previously this was done to add the padding.
3. Attila is going to prototype the Impala-side changes to expose CHAR/VARCHAR and test them before merging this patch, to avoid any surprises down the line. He's also going to investigate whether there are any Impala CHAR/VARCHAR tests that we can easily repurpose for Kudu so as to benefit from the various truncation/padding scenarios.
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.h@44
PS19, Line 44: <==>
What's this mean?
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.h@54
PS19, Line 54: std::string UTF8Resize(Slice val, size_t utf8_length);
For simplicity, could UTF8Truncate be folded into UTF8Resize? Meaning, could UTF8Resize just do the truncation if utf8_length < val.size()? The two functions seem like different halves of the same problem.
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.cc@44
PS19, Line 44: Slice UTF8Truncate(Slice val, size_t max_utf8_length) {
Should DCHECK that max_utf8_length < val.size()?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Tue, 16 Jul 2019 23:13:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 24:
(1 comment)
> Patch Set 24: Code-Review+2
>
> (1 comment)
>
> Looks good to me, though let's hold off merging until you've tested this with Impala.
http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h@153
PS23, Line 153: /// @note The copying behavior is new for these methods starting Kudu 0.10.
: /// Prior to Kudu 0.10, these methods behaved like
: /// KuduPartialRow::SetStringNoCopy() and KuduPartialRow::SetBinaryNoCopy()
: /// correspondingly.
> Nah, not worth it. Let's just skip the NoCopy variants for now; we can alwa
Done
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Tue, 23 Jul 2019 21:15:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#21).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
BINARY.
The maximum length for VARCHAR is 65,535 and 255 for CHAR. Both of them
are truncated to the length of the column and trailing spaces left after
truncation to length are also removed from CHAR fields.
Truncation happens before persisting the data on the server side to
prevent wasting space and also for the predicates to be applied correctly.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 323 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/21
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 22:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13760/22//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13760/22//COMMIT_MSG@15
PS22, Line 15: The maximum length for VARCHAR is 65,535 and 255 for CHAR
Could you add _why_ these maximum lengths make sense?
http://gerrit.cloudera.org:8080/#/c/13760/22/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/22/src/kudu/common/partial_row.h@415
PS22, Line 415: /// Get the string/binary value for a column by its name.
This sentence should also be updated.
http://gerrit.cloudera.org:8080/#/c/13760/22/src/kudu/common/partial_row.h@438
PS22, Line 438: /// Get the string/binary value for a column by its index.
Likewise.
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/schema.h@124
PS20, Line 124: // Maximum value of the length is 65,535 for compatibility reasons as it's
: // used by VARCHAR type which can be set to a maximum of 65,535 in case of
: // MySQL and less for other major RDMBMS implementations.
> I believe it should be char quantity, at least in other RDBMSs I used it wa
Thanks for the clarification. Could you update the comment to reflect that the length is a character quantity?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Fri, 19 Jul 2019 15:49:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#11).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
23 files changed, 538 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/11
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 27:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row-test.cc@267
PS26, Line 267: EXPECT_OK(row.Unset(4));
> Does it make sense to add some more cases in here for CHAR:
Done
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row.cc@195
PS26, Line 195:
> nit: drop the semicolon
Done
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row.cc@198
PS26, Line 198: ;
> nit: drop the semicolon
Done
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/schema.h@126
PS26, Line 126: RDBMS
> nit: RDMS ?
Done
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/schema.h@127
PS26, Line 127: characters
> Maybe, explicitly say 'characters/symbols (not bytes)'.
Done
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.h@20
PS26, Line 20: #include <cstddef>
: #include <cstdint>
> nit: why not to use C++ headers cstddef and cstdint ?
Done
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.h@35
PS26, Line 35: constex
> nit: maybe, using constexpr would be more idiomatic here?
Done
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.h@43
PS26, Line 43: TF-8 characters
> Could you add description of the 'max_utf8_length' parameter into the in-li
Done
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.cc@30
PS26, Line 30: (*str & 0xc0) != 0x80
> BTW, where is the UTF-8 validation w.r.t. the number of bytes 2,3,4 as spec
You're right that this doesn't validate the string, documented it.
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.cc@31
PS26, Line 31: num_bytes++
> why in parentheses?
I think it was a pointer at some point when this was in a separate method. Fixed.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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, 27 Aug 2019 17:02:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#17).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
21 files changed, 590 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/17
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 31:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13760/30/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/13760/30/src/kudu/common/common.proto@98
PS30, Line 98: // For varchar columns
> Should remove 'char' here.
Done
http://gerrit.cloudera.org:8080/#/c/13760/30/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/30/src/kudu/common/partial_row.cc@233
PS30, Line 233: case STRING:
: dst = schema_->ExtractColumnFromRow<STRING>(row, col_idx);
: break;
> Nit: might be clearer as:
Done
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 31
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:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 33: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/33/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/33/src/kudu/util/char_util.cc@29
PS33, Line 29: num_utf8_chars += (*str++ & 0xc0) != 0x80;
> This fix is rather concerning given how late in the code review cycle it ma
OK, I see you added tests for this in the "make UTF8Truncate faster" patches. I presume there's enough coverage there, and if not you can add some in those patches.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 33
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 17:03:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#6).
Change subject: [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
[WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
A src/kudu/util/char_util.h
16 files changed, 417 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/6
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 33: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 33
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:44 +0000
Gerrit-HasComments: No
[kudu-CR] [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#2).
Change subject: [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
[WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
A src/kudu/util/char_util.h
16 files changed, 361 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/2
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#15).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
21 files changed, 604 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/15
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 22:
(4 comments)
While double checking maximum lengths for CHAR and VARCHAR in other RDMBSs I noticed that there's a difference between the 'standard' approach of padding CHARs vs. Apache Impala's and now ours.
Originally I implemented the padding of CHARs *before* persisting which it seems is what other databases (e.g. MySQL[1], Oracle[2] and PostgreSQL[3]) is doing. IIRC this was originally to have fixed-width rows, but with UTF-8 they still wouldn't be fixed-width as UTF-8 itself is variable length.
In MySQL's case the trailing spaces are even removed by default when scanned:
> The length of a CHAR column is fixed to the length that you declare when you create the table. The length can be any value from 0 to 255. When CHAR values are stored, they are right-padded with spaces to the specified length. When CHAR values are retrieved, trailing spaces are removed unless the PAD_CHAR_TO_FULL_LENGTH SQL mode is enabled.
Impala[4] on the other hand stores the data without trailing whitespaces and it's padded upon retrieval:
> If you store a CHAR value containing trailing spaces in a table, those trailing spaces are not stored in the data file. When the value is retrieved by a query, the result could have a different number of trailing spaces. That is, the value includes however many spaces are needed to pad it to the specified length of the column.
Due to the variable length nature of UTF8 and the columnar format I believe it makes most sense to implement it the same way as Impala did, only wanted to bring your attention to this discrepancy.
[1] https://docs.oracle.com/cd/E17952_01/mysql-5.1-en/char.html
[2] https://docs.oracle.com/cd/B28359_01/server.111/b28318/datatype.htm#CNCPT1821
[3] https://www.postgresql.org/docs/9.0/datatype-character.html
[4] https://impala.apache.org/docs/build/html/topics/impala_char.html
http://gerrit.cloudera.org:8080/#/c/13760/22//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13760/22//COMMIT_MSG@15
PS22, Line 15: The maximum length for VARCHAR is 65,535 and 255 for CHAR
> Could you add _why_ these maximum lengths make sense?
Done
http://gerrit.cloudera.org:8080/#/c/13760/22/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/22/src/kudu/common/partial_row.h@415
PS22, Line 415: /// Get the string/binary value for a column by its name.
> This sentence should also be updated.
Done
http://gerrit.cloudera.org:8080/#/c/13760/22/src/kudu/common/partial_row.h@438
PS22, Line 438: /// Get the string/binary value for a column by its index.
> Likewise.
Done
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/schema.h@124
PS20, Line 124: // Maximum value of the length is 65,535 for compatibility reasons as it's
: // used by VARCHAR type which can be set to a maximum of 65,535 in case of
: // MySQL and less for other major RDMBMS implementations.
> Thanks for the clarification. Could you update the comment to reflect that
Done
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Sat, 20 Jul 2019 10:37:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 16:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/13760/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13760/16//COMMIT_MSG@10
PS16, Line 10: Follow up commits will add integration to other clients. The CHAR and
Mind separating the C++ client piece into a separate commit too? I'd like to review the server side pieces without any distractions if it's not much work.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.h@148
PS16, Line 148: /// @name Setters for binary/string columns by name (copying).
Update.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.h@171
PS16, Line 171: /// @name Setters for binary/string columns by index (copying).
Update.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.h@432
PS16, Line 432: Status GetCharVarchar(const Slice& col_name, Slice* val) const WARN_UNUSED_RESULT;
Shouldn't we have separate GetChar and GetVarchar getters, as you did for setters?
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc@239
PS16, Line 239: if (col.type_info()->type() == BINARY) {
: dst = schema_->ExtractColumnFromRow<BINARY>(row, col_idx);
: } else if (col.type_info()->type() == CHAR) {
: dst = schema_->ExtractColumnFromRow<CHAR>(row, col_idx);
: } else if (col.type_info()->type() == VARCHAR) {
: dst = schema_->ExtractColumnFromRow<VARCHAR>(row, col_idx);
: } else {
: CHECK(col.type_info()->type() == STRING);
: dst = schema_->ExtractColumnFromRow<STRING>(row, col_idx);
: }
Convert into a switch.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc@440
PS16, Line 440: size_t length = col.type_attributes().length;
This is only used in one place; consider not using a local for it.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc@441
PS16, Line 441: resize
It's late but I have no idea what resize() is.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc@793
PS16, Line 793: decimal
Not decimal
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/schema.h@123
PS16, Line 123: uint16_t length;
Why uint16_t? What does this implicitly say about the maximum length of a CHAR or VARCHAR?
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.h@28
PS16, Line 28: namespace kudu {
These function names are a little too generic (especially resize). Plus they should use CamelCase naming.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.h@38
PS16, Line 38: /// Check the UTF8 and character length of the slice up to a maximum length.
This isn't part of the public API, so you don't need to use Doxygen style comments.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.h@60
PS16, Line 60: bool expand
Define a two-state enum instead; it'll be more readable.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.cc@27
PS16, Line 27: void utf8length(const Slice& slice, size_t* utf8_length,
Does this duplicate functionality from gutil/utf?
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.cc@46
PS16, Line 46: char_length += length - utf8_length;
It's a little confusing as to why we need three different kinds of length here. Could you add some comments?
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.cc@66
PS16, Line 66: std::s
Add a "using std::string" at the top and drop this.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Thu, 11 Jul 2019 05:52:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
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/13760
to look at the new patch set (#31).
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
KUDU-1938 Add support for VARCHAR pt 1
Introduces the VARCHAR data type to the server. Follow up commits will
add integration to the clients. The VARCHAR type is parameterized with a
length column type attribute similar to DECIMAL's scale and precision.
Internally it's stored as BINARY.
The maximum length for VARCHAR is 65,535 characters. If a value longer
than the "n" is submitted for a VARCHAR(n) column the value is truncated
to "n" characters before persisting the data on the server side.
The maximum length was chosen for compatibility reasons. Apache Impala
has a maximum length of 65,535 *bytes* for VARCHAR and major RDBMSs I
checked also have a similar limits either in characters or bytes, mostly
configurable.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 259 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/31
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 31
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 support for VARCHAR pt 1
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 33: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 33
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 08:22:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 18:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/common/partial_row.h@150
PS17, Line 150: /// Set the binary/string/char/varchar value for a column by name, copying the
: /// specified data im
> This too (and in the other variant below).
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/schema.h@123
PS16, Line 123:
> Makes sense, we just need to make sure it's documented in the code where ap
Done
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@28
PS17, Line 28: namespace kudu {
> FYI, slices are cheap enough that when we need to pass them read-only in ne
Done
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@40
PS17, Line 40: s, the number of
: // bytes in num_bytes.
> This is confusing: if you want to store a byte quantity, name the parameter
Renamed it, but it's necessary for the truncate/resize methods to know both the number of bytes and the UTF8 characters at the same offset.
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@41
PS17, Line 41: e max_utf8_length parameter determines the max
: // number of UTF8 characters to be read to
> Is this optimization worth making? Are we super concerned about counting to
It's not really an optimization as much as a necessity to stop counting here so the num_bytes would align with num_utf8_chars
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@46
PS17, Line 46: void UTF8Length(const Slice val, size_t* num_utf8_chars,
> Does 'length' indicate the new slice length? Is it a byte quantity or a UTF
Changed it to max_utf8_length, should be clearer now. It doesn't need to be less than val.length(), we only use it as a maximum. The first n bytes correlating to the firs max_utf8_length characters of the val.data() are copied to a new location as it's used in KuduPartialRow::SetCharVarchar where it's expected to memcpy (i.e. SetString, SetBinary both call SetSliceCopy which uses memcpy as well).
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@50
PS17, Line 50: Slice UTF8Truncate(const Slice val, size_t max_utf8_length);
> Why does this return an std::string?
It returns an std::string because it's used in the scanner. The server serializes all binary type columns to a single string (indirect_data) and stores the offsets and sizes in the fixed width data (direct_data). Then this is passed through the wire and on the client side these offsets and sizes are converted into Slices pointing to locations within this single string.
Due to this there's no way to resize the slices in the getters. Copying data to new slices is also impossible as the Slice doesn't manage the memory of the data it points to so it has to be freed up manually after the client no longer needs it.
It should be possible to copy all data with the necessary padding after the message is received in `RewriteRowBlockPointers`, but that would require iterating through all rows at least one more time, appending every single binary data to a new faststring/std::string.
First I opted to do the CHAR expansion in the sending end of the wire when the data is serialized in `SerializeRowBlock`. This way the receiving end doesn't have to do any extra work (in addition to saving on the memcpy and extra iteration on the rows, less complex code on the client side) and the extra spaces are still not persisted on the disk and the predicates are applied correctly too. The expansion applies only to CHARs anyway which are max 255 chars now as you suggested and the client side would've had to rewrite all binary data even if there's a single CHAR column in the projection.
I then realized it would be much simpler to use std::string instead of Slice here so the allocated memory doesn't get freed up before the client could read it and it wouldn't break existing API as GetChar is a new method, so this is why this is also returning an std::string. I could dig out my previous approach from reflog if that's better.
> What does it mean for the memory pointed to by 'val'?
It leaves it unchanged.
> Is 'length' a byte quantity or a UTF8 character quantity?
It's UTF8, changed the parameter name to reflect this.
> Can it be less than val.length()?
It can, it would truncate in this case. Hopefully this is also clear now from the name of the parameter.
> If the string is expanded, is the excess padded?
Yes, added a comment to explain it.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.cc@27
PS16, Line 27:
> Does this duplicate functionality from gutil/utf?
There is a similar method [utfnlen(const char*, long n)], but the n represents the maximum number of bytes instead of maximum number of UTF8 characters + it doesn't set char_length (renamed to num_bytes) which we need in resize/truncate
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.cc@46
PS16, Line 46: size_t num_bytes = 0;
> It's a little confusing as to why we need three different kinds of length h
Removed this part from here as I realized we don't need this, but explained it in the resize_to_str (renamed to UTF8Resize) + renamed the variables as well to make more sense.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.cc@66
PS16, Line 66:
> Add a "using std::string" at the top and drop this.
Done
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
PS17:
> Looks like you missed the review comments in this file.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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, 16 Jul 2019 05:13:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 24:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13760/23//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13760/23//COMMIT_MSG@23
PS23, Line 23: 65,535
> Did you mean 65535 here?
Done
http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h@153
PS23, Line 153: /// @note The copying behavior is new for these methods starting Kudu 0.10.
: /// Prior to Kudu 0.10, these methods behaved like
: /// KuduPartialRow::SetStringNoCopy() and KuduPartialRow::SetBinaryNoCopy()
: /// correspondingly.
> This @note doesn't apply to the new methods you added (below too). Perhaps
split them. I don't think we can get away with NoCopy variants here as we're expecting const Slice& values and to truncate we need to modify their size. Maybe if we would expect pointers, but that seems way too different from the rest of the API. A middle ground would be to instead of truncating we would only verify the length and that there's no extra padding in case of CHARs and retutrn Status::InvalidArgument if it's invalid?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Tue, 23 Jul 2019 14:05:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 20:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.h@413
PS20, Line 413: /// @name Getters for string/binary/char/varchar column by column name.
Do we want to doc that the CHAR getters do _not_ pad with whitespace?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Wed, 17 Jul 2019 17:15:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#16).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
21 files changed, 607 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/16
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#9).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/util/char_util.h
19 files changed, 477 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/9
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#20).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 326 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/20
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for CHAR/VARCHAR pt 1
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc@1124
PS10, Line 1124: ColumnSchema chars("chars", CHAR);
> I changed the name, 'char' is a reserved keyword, so I can't rename the var
ah, right.
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row-test.cc@270
PS13, Line 270: EXPECT_EQ("char char_val=\"shortval \"", row.ToString());
Should char values be padded in row.ToString()?
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row.cc@432
PS13, Line 432: Status KuduPartialRow::SetCharVarchar(const Slice& col_name, const Slice& val) {
Maybe move this truncating logic to char_util.cc?
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h@634
PS10, Line 634: struct DataTypeTraits<CHAR> : public DerivedTypeTraits<BINARY>{
> DerivedTypeTraits is template<DataType PhysicalType>, so I think only physi
I forgot STRING was already a "DerivedType". This makes sense.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h@647
PS10, Line 647: struct DataTypeTraits<VARCHAR> : public DerivedTypeTraits<BINARY>{
> Can this be DerivedTypeTraits STRING to unify all the UTF8 and debug string
Done
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/util/slice.h
File src/kudu/util/slice.h:
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/util/slice.h@176
PS13, Line 176: std::string ToString() const;
Should this live in char_util.cc instead of slice?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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: Sat, 29 Jun 2019 20:44:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 23:
(1 comment)
> Patch Set 22:
>
> Due to the variable length nature of UTF8 and the columnar format I believe it makes most sense to implement it the same way as Impala did, only wanted to bring your attention to this discrepancy.
This is all super useful design trade off information. Could you try to summarize it into the commit message?
http://gerrit.cloudera.org:8080/#/c/13760/23//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13760/23//COMMIT_MSG@23
PS23, Line 23: 65,353
Did you mean 65535 here?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Mon, 22 Jul 2019 19:01:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR pt 1
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/13760
to look at the new patch set (#29).
Change subject: KUDU-1938 Add support for CHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR pt 1
Introduces the CHAR data type to the server. Follow up commits will add
integration to the clients. The CHAR type is parameterized with a length
column type attribute similar to DECIMAL's scale and precision.
Internally it's stored as BINARY.
The maximum length for VARCHAR is 65,535 characters. If a value longer
than the "n" is submitted for a VARCHAR(n) column the value is truncated
to "n" characters before persisting the data on the server side.
The maximum length was chosen for compatibility reasons. Apache Impala
has a maximum length of 65,535 *bytes* for VARCHAR and major RDBMSs I
checked also have a similar limits either in characters or bytes, mostly
configurable.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 258 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/29
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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] [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#3).
Change subject: [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
[WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
A src/kudu/util/char_util.h
16 files changed, 363 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/3
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 20:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.h@413
PS20, Line 413: /// @name Getters for string/binary/char/varchar column by column name.
> Do we want to doc that the CHAR getters do _not_ pad with whitespace?
I think the method should be GetUnpadedChar so we can add a GetChar that does padding if we want later. This matches GetUnscaledDecimal.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Wed, 17 Jul 2019 17:17:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 28: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Fri, 30 Aug 2019 16:35:33 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 31: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/31/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/31/src/kudu/util/char_util.cc@24
PS31, Line 24: Slice UTF8Truncate(Slice val, size_t max_utf8_length) {
clang-tidy says you can remove last_non_whitespace.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 31
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:16:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 32: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 32
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 22:16:23 +0000
Gerrit-HasComments: No
[kudu-CR] [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#5).
Change subject: [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
[WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
A src/kudu/util/char_util.h
15 files changed, 396 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/5
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#8).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/util/char_util.h
19 files changed, 477 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/8
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 17:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13760/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13760/16//COMMIT_MSG@10
PS16, Line 10: Follow up commits will add integration to other clients. The CHAR and
> Mind separating the C++ client piece into a separate commit too? I'd like t
Sure, it was pretty straight-forward actually. One thing I'm unsure of is whether partial_row should be considered server or client in this case as it's in common.
Also, should I abandon this change and create two separate commits or just drop everything non-server code from this commit by rebasing it and and commit them in a follow-up commit?
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.h@148
PS16, Line 148: /// @name Setters for binary/string/char/varchar columns by name (copying).
> Update.
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.h@171
PS16, Line 171: /// @name Setters for binary/string/char/varchar columns by index (copying).
> Update.
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.h@432
PS16, Line 432: Status GetChar(const Slice& col_name, Slice* val) const WARN_UNUSED_RESULT;
> Shouldn't we have separate GetChar and GetVarchar getters, as you did for s
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc@239
PS16, Line 239: switch (col.type_info()->type()) {
: case BINARY:
: dst = schema_->ExtractColumnFromRow<BINARY>(row, col_idx);
: break;
: case CHAR:
: dst = schema_->ExtractColumnFromRow<CHAR>(row, col_idx);
: break;
: case VARCHAR:
: dst = schema_->ExtractColumnFromRow<VARCHAR>(row, col_idx);
:
> Convert into a switch.
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc@440
PS16, Line 440: Substitute("$0 expected, $1 given", col.type_info()->name(), T::name()));
> This is only used in one place; consider not using a local for it.
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc@441
PS16, Line 441:
> It's late but I have no idea what resize() is.
That's a custom UTF8 resize from char_util
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/partial_row.cc@793
PS16, Line 793:
> Not decimal
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/schema.h@123
PS16, Line 123: uint16_t length;
> Why uint16_t? What does this implicitly say about the maximum length of a C
Will or Grant suggested the max size to be 65,535 characters for VARCHAR for compatibility reasons. It seems this limit is 65,535 in MySQL and lower in all other mainstream RDBMSs so I went with it to be on the safe side. uint16_t's max value is conveniently 65,535 so I used it here.
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.h@28
PS16, Line 28: namespace kudu {
> These function names are a little too generic (especially resize). Plus the
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.h@38
PS16, Line 38: // Get the number of UTF8 characters of a slice
> This isn't part of the public API, so you don't need to use Doxygen style c
Done
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/util/char_util.h@60
PS16, Line 60:
> Define a two-state enum instead; it'll be more readable.
I realized this isn't even necessary at this point due to some refactoring
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Mon, 15 Jul 2019 13:43:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 32:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/31/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/31/src/kudu/util/char_util.cc@24
PS31, Line 24: Slice UTF8Truncate(Slice val, size_t max_utf8_length) {
> clang-tidy says you can remove last_non_whitespace.
clang-tidy was right, thanks. this was relevant for chars only
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 32
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:31:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#25).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
BINARY.
The maximum length for VARCHAR is 65,535 and 255 for CHAR. Both of them
are truncated to the length of the column and trailing spaces left after
truncation to length are also removed from CHAR fields.
Truncation happens before persisting the data on the server side to
prevent wasting space and also for the predicates to be applied correctly.
The maximum lengths were chosen for compatibility reasons. Apache Impala
has a maximum length of 255 characters for CHAR and 65,535 for VARCHAR,
major RDBMSs I checked also had similar, or lower limits.
There's a difference between the 'standard' approach used by traditional
RDBMSs of padding CHARs vs. Apache Impala's and now ours.
Originally I implemented the padding of CHARs *before* persisting which
it seems is what other databases (e.g. MySQL[1], Oracle[2] and
PostgreSQL[3]) is doing. IIRC this was originally to have fixed-width
rows, but with UTF-8 they still wouldn't be fixed-width as UTF-8 itself
is variable length.
In MySQL's case the trailing spaces are even removed by default when
scanned:
> The length of a CHAR column is fixed to the length that you declare
> when you create the table. The length can be any value from 0 to 255.
> When CHAR values are stored, they are right-padded with spaces to the
> specified length. When CHAR values are retrieved, trailing spaces are
> removed unless the PAD_CHAR_TO_FULL_LENGTH SQL mode is enabled.
Impala[4] on the other hand stores the data without trailing whitespaces
and it's padded upon retrieval:
> If you store a CHAR value containing trailing spaces in a table, those
> trailing spaces are not stored in the data file. When the value is
> retrieved by a query, the result could have a different number of
> trailing spaces. That is, the value includes however many spaces are
> needed to pad it to the specified length of the column.
Due to the variable length nature of UTF8 and the columnar format I
believe it makes most sense to implement it the same way as Impala did.
[1] https://docs.oracle.com/cd/E17952_01/mysql-5.1-en/char.html
[2]
https://docs.oracle.com/cd/B28359_01/server.111/b28318/datatype.htm#CNCPT1821
[3] https://www.postgresql.org/docs/9.0/datatype-character.html
[4] https://impala.apache.org/docs/build/html/topics/impala_char.html
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 357 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/25
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 32: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 32
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:04:00 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#19).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 349 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/19
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 16:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc@1124
PS10, Line 1124: ColumnSchema chars("char", CHAR);
> ah, right.
Done
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row-test.cc@270
PS13, Line 270: EXPECT_EQ("char char_val=\"shortval\"", row.ToString());
> Should char values be padded in row.ToString()?
I don't think we can do that without storing the value with padding. We would need to have the length which isn't available in the physical SLICE type.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.cc@455
PS10, Line 455: Status KuduPartialRow::SetNull(int col_idx) {
> I think we should follow the same semantics as Impala: https://impala.apach
Done
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row.cc@432
PS13, Line 432: if (PREDICT_FALSE(T::type != col.type_info()->type())) {
> Maybe move this truncating logic to char_util.cc?
Done
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h@634
PS10, Line 634: struct DataTypeTraits<CHAR> : public DerivedTypeTraits<BINARY>{
> I forgot STRING was already a "DerivedType". This makes sense.
Done
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/util/slice.h
File src/kudu/util/slice.h:
http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/util/slice.h@176
PS13, Line 176: std::string ToString() const;
> Should this live in char_util.cc instead of slice?
Done
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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: Sun, 30 Jun 2019 21:27:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#13).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
23 files changed, 540 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/13
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#10).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server and C++ client.
Follow up commits will add integration to other clients. The CHAR and
VARCHAR types are parameterized with a length column type attribute
similar to DECIMAL's scale and precision. Internally both of them are
stored as slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
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/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/util/char_util.h
19 files changed, 478 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/10
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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 support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#22).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
BINARY.
The maximum length for VARCHAR is 65,535 and 255 for CHAR. Both of them
are truncated to the length of the column and trailing spaces left after
truncation to length are also removed from CHAR fields.
Truncation happens before persisting the data on the server side to
prevent wasting space and also for the predicates to be applied correctly.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 323 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/22
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 19:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.h@44
PS19, Line 44: <==>
> What's this mean?
tried to denote "if and only if" with this. Deleting this method anyway for now as it's easier to do this whole thing as part of UTF8Truncate and it's not used anywhere else now anyway without UTF8Resize.
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.h@54
PS19, Line 54: std::string UTF8Resize(Slice val, size_t utf8_length);
> For simplicity, could UTF8Truncate be folded into UTF8Resize? Meaning, coul
yeah I think I originally I split them because I needed an std::string in one case and Slice in the other. I think we don't even need this as we're not planning to pad, only to truncate, so I'll go ahead and remove this method for now.
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/19/src/kudu/util/char_util.cc@44
PS19, Line 44: Slice UTF8Truncate(Slice val, size_t max_utf8_length) {
> Should DCHECK that max_utf8_length < val.size()?
I don't think we need to, calling this method with max_utf8_length >= val.size() should be valid, in which case it's not truncated, only copied. At least that's how I use it right now as we need to memcpy val.data() either way.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Wed, 17 Jul 2019 16:35:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 33:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/33/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/33/src/kudu/util/char_util.cc@29
PS33, Line 29: num_utf8_chars += (*str++ & 0xc0) != 0x80;
> OK, I see you added tests for this in the "make UTF8Truncate faster" patche
Actually this was a result of a botched refactor. When I removed the code relevant to CHAR type I mess it up and removed the code which advanced this iterator. If you compare it to PS31 you'll see that in L37 there was an additional condition where the iterator was advanced. When I removed that part as it wasn't needed anymore I forgot to change *str to *str++ here.
There are tests in partial_row-test.cc which caught this. There are also more test coverage in follow-up commits as you noted.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 33
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 19:16:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
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/13760
to look at the new patch set (#30).
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
KUDU-1938 Add support for VARCHAR pt 1
Introduces the VARCHAR data type to the server. Follow up commits will
add integration to the clients. The VARCHAR type is parameterized with a
length column type attribute similar to DECIMAL's scale and precision.
Internally it's stored as BINARY.
The maximum length for VARCHAR is 65,535 characters. If a value longer
than the "n" is submitted for a VARCHAR(n) column the value is truncated
to "n" characters before persisting the data on the server side.
The maximum length was chosen for compatibility reasons. Apache Impala
has a maximum length of 65,535 *bytes* for VARCHAR and major RDBMSs I
checked also have a similar limits either in characters or bytes, mostly
configurable.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 258 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/30
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 13:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/scan_batch.h@266
PS10, Line 266: Status GetChar(const Slice& col_name, Slice* val) const WARN_UNUSED_RESULT;
> Similar to GetBinary and GetString, I think these should be different metho
Done
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/schema.cc@348
PS10, Line 348: } else if (data_->type.value() == KuduColumnSchema::CHAR) {
> I think the standard max CHAR length is 254.
Done
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/client/schema.cc@377
PS10, Line 377: strings::Substitute("length is not valid on a $0 column",
> This should likely default to 0 given it's "unused" in this case. That matc
Done
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc@1124
PS10, Line 1124: ColumnSchema chars("char", CHAR);
> nit: "char"
I changed the name, 'char' is a reserved keyword, so I can't rename the variable
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.h@167
PS10, Line 167: Status SetChar(const Slice& col_name, const Slice& val) WARN_UNUSED_RESULT;
> Similar to SetBinary and SetString, I think these should be different metho
Done
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h@634
PS10, Line 634: struct DataTypeTraits<CHAR> : public DerivedTypeTraits<BINARY>{
> Can this be DerivedTypeTraits STRING to unify all the UTF8 and debug string
DerivedTypeTraits is template<DataType PhysicalType>, so I think only physical types can be used here. When I tried changing it to STRING it compiled, but the all_types-itest failed for each char type with this:
"Bad status: Not implemented: Error creating table all-types-table on the master: invalid encoding for column 'char_val': encoding AUTO_ENCODING not supported for type STRING"
Maybe I'm just missing something (do you have any pointers in this case?), but I think extending DerivedTypeTraits<STRING> won't work.
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/util/char_util.h@28
PS10, Line 28:
> I think the standard for max CHAR length is 255 and max VARCHAR is 65535.
Done
http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/util/char_util.h@29
PS10, Line 29: // Minimum and maximum length for CHAR [1,255]
> Do we need to have a default? Shouldn't we require it to be specified fo CH
Done
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: 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: Sat, 29 Jun 2019 18:58:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
KUDU-1938 Add support for VARCHAR pt 1
Introduces the VARCHAR data type to the server. Follow up commits will
add integration to the clients. The VARCHAR type is parameterized with a
length column type attribute similar to DECIMAL's scale and precision.
Internally it's stored as BINARY.
The maximum length for VARCHAR is 65,535 characters. If a value longer
than the "n" is submitted for a VARCHAR(n) column the value is truncated
to "n" characters before persisting the data on the server side.
The maximum length was chosen for compatibility reasons. Apache Impala
has a maximum length of 65,535 *bytes* for VARCHAR and major RDBMSs I
checked also have a similar limits either in characters or bytes, mostly
configurable.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Reviewed-on: http://gerrit.cloudera.org:8080/13760
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 256 insertions(+), 28 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
Grant Henke: Looks good to me, approved
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 34
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 support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 25: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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, 13 Aug 2019 06:24:56 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 20:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13760/20//COMMIT_MSG
Commit Message:
PS20:
I'd use the commit message to document the expected padding/truncation semantics, as well as a discussion of trade-offs (i.e. why are we truncating before we store the data server-side?)
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row-test.cc@290
PS20, Line 290: s = row.SetVarchar("varchar_val", "shortval");
Also add a VARCHAR test that proves that it doesn't truncate excess whitespace.
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.cc@423
PS20, Line 423: case CHAR:
: relocated_val = UTF8Truncate(val, col.type_attributes().length,
: TRUNCATE_WHITESPACE);
: break;
: case VARCHAR:
: relocated_val = UTF8Truncate(val, col.type_attributes().length,
: NO_TRUNCATE_WHITESPACE);
: break;
Maybe rewrite like this:
case CHAR:
case VARCHAR:
relocated_val = UTF8Truncate(val, col.type_attributes().length,
T::type == CHAR ? TRUNCATE_WHITESPACE : NO_TRUNCATE_WHITESPACE);
break;
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/schema.h@124
PS20, Line 124: // Maximum value of the length is 65,535 for compatibility reasons as it's
: // used by VARCHAR type which can be set to a maximum of 65,535 in case of
: // MySQL and less for other major RDMBMS implementations.
Did you consider limiting it to 255 in order to be exactly compatible with Impala? Looks like Impala will truncate excess characters if the underlying storage stores more than 255. Do we care?
Also, I'm trying to figure out whether the length should refer to a _byte quantity_ or a _char quantity_ (the two aren't the same in UTF-8). The Impala docs are a little ambiguous here.
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.h@30
PS20, Line 30: enum TruncateType {
: NO_TRUNCATE_WHITESPACE,
: TRUNCATE_WHITESPACE,
: };
I might rename this:
enum class TrailingWhitespace {
IGNORE,
TRUNCATE
};
By making it an enum class, references to it have to be fully qualified (i.e. TrailingWhitespace::IGNORE). This makes it perfectly clear what the effect will be.
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.h@43
PS20, Line 43: // Copy and truncate a slice
Should also say that the returned Slice "owns" its memory (which is somewhat unusual for a Slice, hence the need to doc it).
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.cc@45
PS20, Line 45: / sizeof(uint8_t)
Not necessary: isn't this always "divide by 1"?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Wed, 17 Jul 2019 17:14:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 17:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/13760/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13760/16//COMMIT_MSG@10
PS16, Line 10: Follow up commits will add integration to other clients. The CHAR and
> Sure, it was pretty straight-forward actually. One thing I'm unsure of is w
Put PartialRow in the server change.
Keep this change as the server-side change (so we can preserve the review comments) and make a new change for the client stuff.
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/common/partial_row.h@150
PS17, Line 150: /// Set the binary/string value for a column by name, copying the specified
: /// data immediately.
This too (and in the other variant below).
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/16/src/kudu/common/schema.h@123
PS16, Line 123: uint16_t length;
> Will or Grant suggested the max size to be 65,535 characters for VARCHAR fo
Makes sense, we just need to make sure it's documented in the code where applicable (i.e. not just in the client APIs, but also server-side for other developers).
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@28
PS17, Line 28: namespace kudu {
FYI, slices are cheap enough that when we need to pass them read-only in new code we just pass them by value.
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@40
PS17, Line 40: the number of
: // bytes in char_length
This is confusing: if you want to store a byte quantity, name the parameter "num_bytes" or some such. But why even bother; isn't the number of bytes is accessible via slice.length()? Or is it necessary given how max_utf8_length works?
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@41
PS17, Line 41: The max_utf8_length parameter determines the max
: // number of UTF8 characters to be counted.
Is this optimization worth making? Are we super concerned about counting too much?
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@46
PS17, Line 46: // Copy and truncate a slice
Does 'length' indicate the new slice length? Is it a byte quantity or a UTF8 character quantity? Must it be less than val.length()? What is being "copied" exactly; as far as I can tell we're just generating a new Slice and returning it/
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.h@50
PS17, Line 50: std::string UTF8Resize(const Slice& val, size_t length);
Why does this return an std::string? What does it mean for the memory pointed to by 'val'? Is 'length' a byte quantity or a UTF8 character quantity? Can it be less than val.length()? If the string is expanded, is the excess padded?
http://gerrit.cloudera.org:8080/#/c/13760/17/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
PS17:
Looks like you missed the review comments in this file.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Mon, 15 Jul 2019 21:21:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#28).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
BINARY.
The maximum length for VARCHAR is 65,535 and 255 for CHAR. Both of them
are truncated to the length of the column and trailing spaces left after
truncation to length are also removed from CHAR fields.
Truncation happens before persisting the data on the server side to
prevent wasting space and also for the predicates to be applied correctly.
The maximum lengths were chosen for compatibility reasons. Apache Impala
has a maximum length of 255 characters for CHAR and 65,535 for VARCHAR,
major RDBMSs I checked also had similar, or lower limits.
There's a difference between the 'standard' approach used by traditional
RDBMSs of padding CHARs vs. Apache Impala's and now ours.
Originally I implemented the padding of CHARs *before* persisting which
it seems is what other databases (e.g. MySQL[1], Oracle[2] and
PostgreSQL[3]) is doing. IIRC this was originally to have fixed-width
rows, but with UTF-8 they still wouldn't be fixed-width as UTF-8 itself
is variable length.
In MySQL's case the trailing spaces are even removed by default when
scanned:
> The length of a CHAR column is fixed to the length that you declare
> when you create the table. The length can be any value from 0 to 255.
> When CHAR values are stored, they are right-padded with spaces to the
> specified length. When CHAR values are retrieved, trailing spaces are
> removed unless the PAD_CHAR_TO_FULL_LENGTH SQL mode is enabled.
Impala[4] on the other hand stores the data without trailing whitespaces
and it's padded upon retrieval:
> If you store a CHAR value containing trailing spaces in a table, those
> trailing spaces are not stored in the data file. When the value is
> retrieved by a query, the result could have a different number of
> trailing spaces. That is, the value includes however many spaces are
> needed to pad it to the specified length of the column.
Due to the variable length nature of UTF8 and the columnar format I
believe it makes most sense to implement it the same way as Impala did.
[1] https://docs.oracle.com/cd/E17952_01/mysql-5.1-en/char.html
[2]
https://docs.oracle.com/cd/B28359_01/server.111/b28318/datatype.htm#CNCPT1821
[3] https://www.postgresql.org/docs/9.0/datatype-character.html
[4] https://impala.apache.org/docs/build/html/topics/impala_char.html
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 390 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/28
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for VARCHAR pt 1
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/13760
to look at the new patch set (#33).
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
KUDU-1938 Add support for VARCHAR pt 1
Introduces the VARCHAR data type to the server. Follow up commits will
add integration to the clients. The VARCHAR type is parameterized with a
length column type attribute similar to DECIMAL's scale and precision.
Internally it's stored as BINARY.
The maximum length for VARCHAR is 65,535 characters. If a value longer
than the "n" is submitted for a VARCHAR(n) column the value is truncated
to "n" characters before persisting the data on the server side.
The maximum length was chosen for compatibility reasons. Apache Impala
has a maximum length of 65,535 *bytes* for VARCHAR and major RDBMSs I
checked also have a similar limits either in characters or bytes, mostly
configurable.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 256 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/33
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 33
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 support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 27: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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, 27 Aug 2019 22:17:25 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#18).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
slices.
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 349 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/18
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 23:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h@153
PS23, Line 153: /// @note The copying behavior is new for these methods starting Kudu 0.10.
: /// Prior to Kudu 0.10, these methods behaved like
: /// KuduPartialRow::SetStringNoCopy() and KuduPartialRow::SetBinaryNoCopy()
: /// correspondingly.
This @note doesn't apply to the new methods you added (below too). Perhaps we should split out Char and Varchar from the setters here into a separate section to avoid the confusion?
On a related note, do you think we should add NoCopy variants of the new functions?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Mon, 22 Jul 2019 19:09:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 21:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/13760/20//COMMIT_MSG
Commit Message:
PS20:
> I'd use the commit message to document the expected padding/truncation sema
Done
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row-test.cc@290
PS20, Line 290: s = row.SetVarchar("varchar_val", "shortval");
> Also add a VARCHAR test that proves that it doesn't truncate excess whitesp
Done
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.h@413
PS20, Line 413: /// @name Getters for string/binary/char/varchar column by column name.
> I think the method should be GetUnpadedChar so we can add a GetChar that do
Done
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/partial_row.cc@423
PS20, Line 423: case CHAR:
: case VARCHAR:
: relocated_val = UTF8Truncate(val, col.type_attributes().length,
: T::type == CHAR ? TrailingWhitespace::TRUNCATE
: : TrailingWhitespace::IGNORE);
: break;
: case STRING:
: case BIN
> Maybe rewrite like this:
Done
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/common/schema.h@124
PS20, Line 124: // Maximum value of the length is 65,535 for compatibility reasons as it's
: // used by VARCHAR type which can be set to a maximum of 65,535 in case of
: // MySQL and less for other major RDMBMS implementations.
> Did you consider limiting it to 255 in order to be exactly compatible with
I believe it should be char quantity, at least in other RDBMSs I used it was always character length. CHAR is actually 255, only VARCHAR is 65,535 (see kMaxCharLength in char_util.h).
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.h@30
PS20, Line 30: IGNORE,
: TRUNCATE,
: };
:
> I might rename this:
Done
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.h@43
PS20, Line 43: Slice UTF8Truncate(Slice val, size_t max_utf8_length, TrailingWhitespace type);
> Should also say that the returned Slice "owns" its memory (which is somewha
Done
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.cc@22
PS20, Line 22: namespace kudu {
> warning: using decl 'min' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/13760/20/src/kudu/util/char_util.cc@45
PS20, Line 45: o it.
> Not necessary: isn't this always "divide by 1"?
Done
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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: Fri, 19 Jul 2019 13:02:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for VARCHAR pt 1
......................................................................
Patch Set 30: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13760/30/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/13760/30/src/kudu/common/common.proto@98
PS30, Line 98: // For char/varchar columns
Should remove 'char' here.
http://gerrit.cloudera.org:8080/#/c/13760/30/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/30/src/kudu/common/partial_row.cc@233
PS30, Line 233: case STRING:
: default:
: CHECK(col.type_info()->type() == STRING);
Nit: might be clearer as:
case STRING:
...
break;
default:
LOG(FATAL) << "Unexpected type " << col.type_info()->type();
break;
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Tue, 01 Oct 2019 19:01:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#24).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
BINARY.
The maximum length for VARCHAR is 65,535 and 255 for CHAR. Both of them
are truncated to the length of the column and trailing spaces left after
truncation to length are also removed from CHAR fields.
Truncation happens before persisting the data on the server side to
prevent wasting space and also for the predicates to be applied correctly.
The maximum lengths were chosen for compatibility reasons. Apache Impala
has a maximum length of 255 characters for CHAR and 65,535 for VARCHAR,
major RDBMSs I checked also had similar, or lower limits.
There's a difference between the 'standard' approach used by traditional
RDBMSs of padding CHARs vs. Apache Impala's and now ours.
Originally I implemented the padding of CHARs *before* persisting which
it seems is what other databases (e.g. MySQL[1], Oracle[2] and
PostgreSQL[3]) is doing. IIRC this was originally to have fixed-width
rows, but with UTF-8 they still wouldn't be fixed-width as UTF-8 itself
is variable length.
In MySQL's case the trailing spaces are even removed by default when
scanned:
> The length of a CHAR column is fixed to the length that you declare
> when you create the table. The length can be any value from 0 to 255.
> When CHAR values are stored, they are right-padded with spaces to the
> specified length. When CHAR values are retrieved, trailing spaces are
> removed unless the PAD_CHAR_TO_FULL_LENGTH SQL mode is enabled.
Impala[4] on the other hand stores the data without trailing whitespaces
and it's padded upon retrieval:
> If you store a CHAR value containing trailing spaces in a table, those
> trailing spaces are not stored in the data file. When the value is
> retrieved by a query, the result could have a different number of
> trailing spaces. That is, the value includes however many spaces are
> needed to pad it to the specified length of the column.
Due to the variable length nature of UTF8 and the columnar format I
believe it makes most sense to implement it the same way as Impala did.
[1] https://docs.oracle.com/cd/E17952_01/mysql-5.1-en/char.html
[2]
https://docs.oracle.com/cd/B28359_01/server.111/b28318/datatype.htm#CNCPT1821
[3] https://www.postgresql.org/docs/9.0/datatype-character.html
[4] https://impala.apache.org/docs/build/html/topics/impala_char.html
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 357 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/24
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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] [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13760
to look at the new patch set (#4).
Change subject: [WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
[WIP] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
A src/kudu/util/char_util.h
15 files changed, 354 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/4
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 27:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row-test.cc@267
PS26, Line 267: EXPECT_OK(row.Unset(4));
> Done
By 'Done' you meant these scenarios are in other changelist, like https://gerrit.cloudera.org/#/c/13869 ?
I could not find them elsewhere? Do you mind posting a pointer to the newly added scenarios? Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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, 30 Aug 2019 04:44:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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/13760
to look at the new patch set (#27).
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
KUDU-1938 Add support for CHAR/VARCHAR pt 1
Introduces the CHAR and VARCHAR data types to the server. Follow up
commits will add integration to the clients. The CHAR and VARCHAR types
are parameterized with a length column type attribute similar to
DECIMAL's scale and precision. Internally both of them are stored as
BINARY.
The maximum length for VARCHAR is 65,535 and 255 for CHAR. Both of them
are truncated to the length of the column and trailing spaces left after
truncation to length are also removed from CHAR fields.
Truncation happens before persisting the data on the server side to
prevent wasting space and also for the predicates to be applied correctly.
The maximum lengths were chosen for compatibility reasons. Apache Impala
has a maximum length of 255 characters for CHAR and 65,535 for VARCHAR,
major RDBMSs I checked also had similar, or lower limits.
There's a difference between the 'standard' approach used by traditional
RDBMSs of padding CHARs vs. Apache Impala's and now ours.
Originally I implemented the padding of CHARs *before* persisting which
it seems is what other databases (e.g. MySQL[1], Oracle[2] and
PostgreSQL[3]) is doing. IIRC this was originally to have fixed-width
rows, but with UTF-8 they still wouldn't be fixed-width as UTF-8 itself
is variable length.
In MySQL's case the trailing spaces are even removed by default when
scanned:
> The length of a CHAR column is fixed to the length that you declare
> when you create the table. The length can be any value from 0 to 255.
> When CHAR values are stored, they are right-padded with spaces to the
> specified length. When CHAR values are retrieved, trailing spaces are
> removed unless the PAD_CHAR_TO_FULL_LENGTH SQL mode is enabled.
Impala[4] on the other hand stores the data without trailing whitespaces
and it's padded upon retrieval:
> If you store a CHAR value containing trailing spaces in a table, those
> trailing spaces are not stored in the data file. When the value is
> retrieved by a query, the result could have a different number of
> trailing spaces. That is, the value includes however many spaces are
> needed to pad it to the specified length of the column.
Due to the variable length nature of UTF8 and the columnar format I
believe it makes most sense to implement it the same way as Impala did.
[1] https://docs.oracle.com/cd/E17952_01/mysql-5.1-en/char.html
[2]
https://docs.oracle.com/cd/B28359_01/server.111/b28318/datatype.htm#CNCPT1821
[3] https://www.postgresql.org/docs/9.0/datatype-character.html
[4] https://impala.apache.org/docs/build/html/topics/impala_char.html
Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/char_util.cc
A src/kudu/util/char_util.h
13 files changed, 361 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/13760/27
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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 support for CHAR/VARCHAR pt 1
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 24: Code-Review+2
(1 comment)
Looks good to me, though let's hold off merging until you've tested this with Impala.
http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h@153
PS23, Line 153: /// @note The copying behavior is new for these methods starting Kudu 0.10.
: /// Prior to Kudu 0.10, these methods behaved like
: /// KuduPartialRow::SetStringNoCopy() and KuduPartialRow::SetBinaryNoCopy()
: /// correspondingly.
> split them. I don't think we can get away with NoCopy variants here as we'r
Nah, not worth it. Let's just skip the NoCopy variants for now; we can always change our mind later.
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
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>
Gerrit-Comment-Date: Tue, 23 Jul 2019 20:37:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1
......................................................................
Patch Set 26:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row-test.cc@267
PS26, Line 267: EXPECT_OK(row.Unset(4));
Does it make sense to add some more cases in here for CHAR:
1) all 10 spaces
2) leading 10 spaces and one non-space
3) just 1 space
4) '<space><non-space>'
5) 9 spaces followed by non-space (both non-unicode and unicode non-space)
6) patterns with symbols encoded in 2 and 3 bytes as well, and mix of those
?
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row.cc@195
PS26, Line 195: ;
nit: drop the semicolon
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/partial_row.cc@198
PS26, Line 198: ;
nit: drop the semicolon
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/schema.h@126
PS26, Line 126: RDMBMS
nit: RDMS ?
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/common/schema.h@127
PS26, Line 127: characters
Maybe, explicitly say 'characters/symbols (not bytes)'.
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.h
File src/kudu/util/char_util.h:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.h@20
PS26, Line 20: #include <stddef.h>
: #include <stdint.h>
nit: why not to use C++ headers cstddef and cstdint ?
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.h@35
PS26, Line 35: static
nit: maybe, using constexpr would be more idiomatic here?
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.h@43
PS26, Line 43: max_utf8_length
Could you add description of the 'max_utf8_length' parameter into the in-line doc?
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.cc@30
PS26, Line 30: (*str & 0xc0) != 0x80
BTW, where is the UTF-8 validation w.r.t. the number of bytes 2,3,4 as specified by byte 1 supposed to happen? Maybe, document that this function doesn't validate the input w.r.t. correctness of the UTF-8 encoding.
http://gerrit.cloudera.org:8080/#/c/13760/26/src/kudu/util/char_util.cc@31
PS26, Line 31: (num_bytes)
why in parentheses?
--
To view, visit http://gerrit.cloudera.org:8080/13760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54
Gerrit-Change-Number: 13760
Gerrit-PatchSet: 26
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, 19 Aug 2019 18:20:37 +0000
Gerrit-HasComments: Yes