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 <[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 21:21:37 +0000 Gerrit-HasComments: Yes
