Will Berkeley has posted comments on this change. Change subject: KUDU-861 Support changing default, storage attributes ......................................................................
Patch Set 13: (12 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: : /** > Does Kudu allow a nullable column to not have a default value (e.g. error o Yes. An API for it was omitted. It'll be fixed by KUDU-1746/KUDU-1747, in review at https://gerrit.cloudera.org/#/c/5132/. 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: bytes = Bytes.fromInt((Integer) value); > PartialRow allows setting a binary column with a ByteBuffer, so that should Done 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: Line 135: ColumnSchema.CompressionAlgorithm.SNAPPY, > We just fixed this, so you can remove this try/finally. Done 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? No, because integer types are sent over the wire as 64-bit integers, so we need to accept large values and then truncate them. PS12, Line 71: NULL > prefer using nullptr instead of NULL Done Line 80: if (write_default_ == NULL) { > I commented on this previously but I think it got lost in the shuffle - Can I tried it out and it doesn't work-- IIRC it segfaults on the dereference, but was a while ago. I do remember clearly that tests failed without the extra branches to cover the nullptr case. Line 494: unordered_set<string>::const_iterator it_names; > move this down to above the second if statement, and initialize it with the Done 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. Done PS12, Line 547: = col_names_.find(col_delta.name) > move this initialization to the line above. Done 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: I agree, and I know Todd does too. If it's OK with you, I'd do that as a separate patch, given it's a mostly-orthogonal change of implementation details and it affects more than just the fields added for KUDU-861. PS12, Line 281: return > returns Done 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 Done -- 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: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
