Todd Lipcon has posted comments on this change. Change subject: KUDU-861 Support changing default, storage attributes ......................................................................
Patch Set 8: (19 comments) didn't make it 100% through the tests, but mostly looking good. http://gerrit.cloudera.org:8080/#/c/4310/8//COMMIT_MSG Commit Message: Line 20: write default values can be changed. Yea, that's the intention. The read/write default is an internal detail that's split exactly for this purpose -- only the 'write' can be altered, and 'read' is used to remember the default value at the time the column was created. The user should only see these exposed as 'default' There's some ancient design doc which lays this out but not sure where it went to. Line 27: I also ran into an issue where empty RLE blocks were failing a CHECK is this covered by a new test? http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/cfile/rle_block.h File src/kudu/cfile/rle_block.h: Line 328: CHECK(pos < num_elems_ || num_elems_ == 0) shouldn't this also be checking that if num_elems_ is 0, then we are seeking to 0? (when do we do this?) http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 3020: ASSERT_STR_CONTAINS(s.ToString(), "no alter operation specified"); can this say "for column 'string_val'" or something to be more easy for the user to fix? Line 3029: ASSERT_STR_CONTAINS(s.ToString(), "cannot support AlterColumn of this type"); this error message sounds like it's saying it can't support altering this type (eg int/float/string) rather than this _operation_. maybe rephrase Line 3082: // test changing a default value (binary column) extra small nit: can you capitalize and punctuate the comments in this test? (i.e. end with a '.' and capitalize the first letter of each sentence). Just makes it a little easier to read. Line 3134: ASSERT_FALSE(col_schema.has_read_default()); shouldn't it still have a read-default? if it's non-nullable and has no read-default, then how can we read it? PS8, Line 3142: value might be too large and we'll still be ok, we just truncate. hrm, this sounds like a bug that might have unintended consequences, or at least surprising results if you specify the wrong type. Could we amend the wire protocol to add an optional field for these values that includes the user-provided type, so we can make sure that it matches what the server thinks? (feel free to file a JIRA and do this in a later patch) http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/client/schema.h File src/kudu/client/schema.h: PS8, Line 352: const const on a return value doesn't make that much sense http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/client/table_alterer-internal.cc File src/kudu/client/table_alterer-internal.cc: PS8, Line 107: uto can you make this 'auto*' so it's more obvious it's a pointer? http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/client/value.cc File src/kudu/client/value.cc: Line 194: Slice x = Slice(reinterpret_cast<uint8_t *>(&int_val_), why not just 'return Slice(...)'? http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/common/common.proto File src/kudu/common/common.proto: PS8, Line 95: optional bytes read_default_value = 3; : optional bytes write_default_value = 4; : given that these aren't exposed separately to users, I dont think they should show up in the wire protocol either http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/common/schema.cc File src/kudu/common/schema.cc: Line 58: // NB: this method should do validation before changing the schema this comment is a little unclear... "should do" sounds like it's a TODO, but here you are doing it. If I understand correctly, maybe say "NB: this method does all validation up-front before making any changes to the schema, so that if we return an error then we are guaranteed to have had no effect" PS8, Line 72: // For now, a read default cannot be changed because it can cause : // undesirable behavior. It should not be set. : if (col_delta.has_read_default) { : return Status::InvalidArgument("read default cannot be changed"); : } yea, I think this should just be removed http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/common/schema.h File src/kudu/common/schema.h: PS8, Line 114: : name(std::move(name)), : has_new_name(false), : has_type(false), : has_nullable(false), : has_read_default(false), : has_write_default(false), : remove_default(false), : has_encoding(false), : has_compression(false), : has_block_size(false) { : maybe now that we are in c++11 land, we should just use initializers with the fields below (less likely to forget one) Another thought is maybe we should use boost::optional<> here for all of these to reduce some boilerplate? Should probably check with Adar whether that's problematic given this code gets linked into the client... I think boost/optional is header-only so should be OK? http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: Line 264: pb->set_name(col_delta.name); should probably clear() the pb first http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/integration-tests/alter_table-randomized-test.cc File src/kudu/integration-tests/alter_table-randomized-test.cc: PS8, Line 74: 0, what does 0 do? does that end up just restoring the server default? http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 1218: if (cur_schema.is_key_column(step.rename_column().old_name())) { can you retain the TODO that says you should be able to rename a key column? I think there's actually a JIRA you can point to Line 1233: return Status::InvalidArgument("cannot alter a key column"); same -- 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: 8 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
