Dan Burkert has posted comments on this change. Change subject: KUDU-861 Support changing default, storage attributes ......................................................................
Patch Set 14: (4 comments) http://gerrit.cloudera.org:8080/#/c/4310/12/src/kudu/common/schema.cc File src/kudu/common/schema.cc: Line 80: if (write_default_ == nullptr) { > I tried it out and it doesn't work-- IIRC it segfaults on the dereference, I was curious about this so I tried it out, I think this works correctly: @@ -67,25 +68,10 @@ Status ColumnSchema::ApplyDelta(const ColumnSchemaDelta& col_delta) { } if (col_delta.has_default) { - if (type_info()->physical_type() == BINARY) { - if (write_default_ == nullptr) { - write_default_ = shared_ptr<Variant>( - new Variant(type_info()->type(), - reinterpret_cast<const void *>(&col_delta.default_value))); - } else { - write_default_->Reset(type_info_->type(), - reinterpret_cast<const void *>(&col_delta.default_value)); - } - } else { - if (write_default_ == nullptr) { - write_default_ = shared_ptr<Variant>( - new Variant(type_info()->type(), - reinterpret_cast<const void *>(col_delta.default_value.data()))); - } else { - write_default_->Reset(type_info_->type(), - reinterpret_cast<const void *>(col_delta.default_value.data())); - } - } + const void* value = type_info()->physical_type() == BINARY ? + reinterpret_cast<const void *>(&col_delta.default_value) : + reinterpret_cast<const void *>(col_delta.default_value.data()); + write_default_ = make_shared<Variant>(type_info()->type(), value); } http://gerrit.cloudera.org:8080/#/c/4310/14/src/kudu/common/schema.cc File src/kudu/common/schema.cc: Line 473: if ((it_names = col_names_.find(name)) == col_names_.end()) { another place where the definition can be hoisted out of the if condition. http://gerrit.cloudera.org:8080/#/c/4310/14/src/kudu/common/wire_protocol.h File src/kudu/common/wire_protocol.h: Line 93: ColumnSchemaDelta ColumnSchemaDeltaFromPB(const ColumnSchemaDeltaPB& pb); Note that the protobuf must outlive the returned ColumnSchemaDelta, since it's borrowing the data field. http://gerrit.cloudera.org:8080/#/c/4310/14/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: Line 349: TEST_F(AlterTableTest, TestAddChangeRemoveColumnDefaultValue) { Could you add a string/binary column to the test, and have an alter table op that includes multiple steps? -- 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: 14 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
