Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8188 )
Change subject: KUDU-2132 Error dropping columns renamed to old key column names ...................................................................... Patch Set 1: (3 comments) Looks good to me, nice catch. Please also solicit a review from Dan. http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/common/schema.h@903 PS1, Line 903: return col - cols_.begin() < num_key_columns_; This seems a little magical to me, though maybe it's because I'm easily confused by iterators and overloaded operators. What about a loop like this? for (int i = 0; i < num_key_columns_; i++) { if (cols_[i].name() == col_name) return true; } return false; http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/integration-tests/alter_table-test.cc@2107 PS1, Line 2107: gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName)); Nit: unique_ptr for new code? http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/master/catalog_manager.cc@1761 PS1, Line 1761: if (builder.is_key_column(step.drop_column().name())) { Is my understanding correct that L1726 initializes the builder with the current schema, thus checking the builder here checks against the current schema _and_ whatever ongoing changes have been made to it? -- To view, visit http://gerrit.cloudera.org:8080/8188 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2 Gerrit-Change-Number: 8188 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 02 Oct 2017 03:33:57 +0000 Gerrit-HasComments: Yes
