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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 11 Jul 2019 05:52:38 +0000 Gerrit-HasComments: Yes
