Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12849 )
Change subject: KUDU-1711: ColumnSchema supports storing column comment ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/12849/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12849/1//COMMIT_MSG@9 PS1, Line 9: This patch intends to support storing column comment in ColumnSchema, One question here (I could probably figure this out but I'm lazy so I'll just ask): Do the column comments end up serialized into every WriteRequestPB? I think currently the client fetches the Schema when it opens a table, and then includes the schema on every write. We might just copy the whole SchemaPB into the write, and log it. If users specify lengthy text comments on their columns this might start to have a noticeable effect on WAL performance, etc. http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h File src/kudu/client/schema-internal.h: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h@117 PS1, Line 117: bool has_comment; > I see this is the pattern used by the rest of this class, but I wonder why I think we had some fear of using boost, which might be incorrect http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h@245 PS1, Line 245: const std::string& comment() const; > Since we can't wrap with boost::optional here (client header), we need to f I think it's reasonable enough to say that "no comment" and empty string are the same http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/master/catalog_manager.cc@1369 PS1, Line 1369: if (col.comment().length() > FLAGS_max_column_comment_length) { we should probably validate that the column is valid UTF8 as well (we can refactor some code out of ValidateIdentifier above) -- To view, visit http://gerrit.cloudera.org:8080/12849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82d3228100c262cc4a5c63eaf696927c88bc8990 Gerrit-Change-Number: 12849 Gerrit-PatchSet: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 26 Mar 2019 04:23:36 +0000 Gerrit-HasComments: Yes
