Dan Burkert has posted comments on this change. Change subject: KUDU-861 Support changing default, storage attributes ......................................................................
Patch Set 12: (13 comments) http://gerrit.cloudera.org:8080/#/c/4310/12/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java File java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java: PS12, Line 133: `newDefault` must not be null or : * else throws {@link IllegalArgumentException}. Does Kudu allow a nullable column to not have a default value (e.g. error on omission)? http://gerrit.cloudera.org:8080/#/c/4310/12/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java: Line 256: } else if (value instanceof byte[]) { PartialRow allows setting a binary column with a ByteBuffer, so that should probably work here as well. http://gerrit.cloudera.org:8080/#/c/4310/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java: PS12, Line 88: pizza I think this is going to fail when you rebase against master. The debug format of string columns was recently changed to include quotes around the value. Line 135: syncClient.deleteTable(tableName); We just fixed this, so you can remove this try/finally. http://gerrit.cloudera.org:8080/#/c/4310/12/src/kudu/common/schema.cc File src/kudu/common/schema.cc: Line 60: if (col_delta.has_default && col_delta.default_value.size() < type_info()->size()) { Should this be a != instead of the inequality? PS12, Line 71: NULL prefer using nullptr instead of NULL Line 80: if (write_default_ == NULL) { I commented on this previously but I think it got lost in the shuffle - Can these if/else trees be collapsed to just the else branch? It seems like Reset should work even if the current value is nullptr. Line 494: unordered_set<string>::const_iterator it_names; move this down to above the second if statement, and initialize it with the declaration. Goal here is to remove all the initialization-in-if-conditions, I find those difficult. Line 497: if ((it_names = col_names_.find(new_name)) != col_names_.end()) { Don't use a local variable here, since it's not referenced again. PS12, Line 547: = col_names_.find(col_delta.name) move this initialization to the line above. http://gerrit.cloudera.org:8080/#/c/4310/12/src/kudu/common/schema.h File src/kudu/common/schema.h: Line 128: std::string new_name; I think it would be cleaner if all of these fields were wrapped in a boost::optional instead of having the associated boolean. PS12, Line 281: return returns http://gerrit.cloudera.org:8080/#/c/4310/12/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: PS12, Line 266: indentation -- To view, visit http://gerrit.cloudera.org:8080/4310 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
