You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/07/11 05:52:38 UTC
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
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