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 <[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: Mon, 15 Jul 2019 13:43:35 +0000 Gerrit-HasComments: Yes
