Will Berkeley has posted comments on this change. Change subject: KUDU-861 Support changing default, storage attributes ......................................................................
Patch Set 8: (27 comments) 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 tha OK, good. I won't go to so much effort explaining myself then. Line 27: I also ran into an issue where empty RLE blocks were failing a CHECK > is this covered by a new test? Sorry, I did a poor job of diagnosing/testing this. Please see a test that repro's without the fix; it's in alter_table-test.cc. 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 seekin Done 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 It does. But the test should capture it as well. Added to the assert. 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 t Done Line 3082: // test changing a default value (binary column) > extra small nit: can you capitalize and punctuate the comments in this test Done 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 rea Good catch. Typo: wrong column being checked. 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 That's the problem- there's no user provided type when doing an alter column. There's only an int64_t in the KuduValue, whether it's an INT8 or INT64. Plus, right now I think you can set the default of an INT64 column to (the reinterpreted bytes of) "aaaaaaaa". A type tag could prevent that. 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 Done 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? Done 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(...)'? Done 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 shou Done http://gerrit.cloudera.org:8080/#/c/4310/8/src/kudu/common/schema.cc File src/kudu/common/schema.cc: Line 20: #include <set> > warning: #includes are not sorted properly [llvm-include-order] Done Line 33: using std::set; > warning: using decl 'set' is unused [misc-unused-using-decls] Done Line 35: using std::unordered_map; > warning: using decl 'unordered_map' is unused [misc-unused-using-decls] Done 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, bu Done 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 Done Line 117: // TODO: include attributes_.ToString() -- need to fix unit tests > warning: missing username/bug in TODO [google-readability-todo] Done 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 t Do you mind if I do that as a follow up? Whichever way this code is improved, could also be done in other places. 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 Done 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: Line 39: using client::KuduClientBuilder; > warning: using decl 'KuduClientBuilder' is unused [misc-unused-using-decls] Done Line 43: using client::KuduInsert; > warning: using decl 'KuduInsert' is unused [misc-unused-using-decls] Done PS8, Line 74: 0, > what does 0 do? does that end up just restoring the server default? Yup. 0 means use a server-side default. Explanation added. Line 274: void RenameColumn(const string& existing_name, string new_name) { > warning: the parameter 'new_name' is copied for each invocation but only us Done Line 530: void AddARangePartition(KuduSchema& schema, KuduTableAlterer* table_alterer) { > warning: non-const reference parameter 'schema', make it const or use a poi Done 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 Good catch. Never meant to remove it. In fact I think I removed the comment because I almost included the features. I tested it a bit and IIRC there were no explosions, but then I decided not to add in since it didn't really belong in this patch and should have specialized testing. Line 1233: return Status::InvalidArgument("cannot alter a key column"); > same 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: 8 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
