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 <[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: Fri, 19 Jul 2019 13:02:24 +0000 Gerrit-HasComments: Yes
