You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2017/09/29 23:46:25 UTC
[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8188
Change subject: KUDU-2132 Error dropping columns renamed to old key column names
......................................................................
KUDU-2132 Error dropping columns renamed to old key column names
Suppose a table has two columns 'a' and 'b', and 'a' is the primary
key. Consider the following sequence of alter table steps, done as
part of one alter table request:
1. Rename the key column 'a' to 'c'.
2. Rename 'b' to 'a', the previous name of the key column.
3. Drop 'a'.
This is a valid sequence, but previously we were rejecting it,
saying that a primary key column was being dropped, even though
that isn't the case.
The problem is that CatalogManager::ApplyAlterSchemaSteps
checked whether a column was a key column by checking by
name against the table's schema before any alter steps were
applied. The fix is to check against the schema with the
previous alter steps applied.
Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
---
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
3 files changed, 22 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/8188/1
--
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: newchange
Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Gerrit-Change-Number: 8188
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Oct 2017 03:33:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8188
to look at the new patch set (#2).
Change subject: KUDU-2132 Error dropping columns renamed to old key column names
......................................................................
KUDU-2132 Error dropping columns renamed to old key column names
Suppose a table has two columns 'a' and 'b', and 'a' is the primary
key. Consider the following sequence of alter table steps, done as
part of one alter table request:
1. Rename the key column 'a' to 'c'.
2. Rename 'b' to 'a', the previous name of the key column.
3. Drop 'a'.
This is a valid sequence, but previously we were rejecting it,
saying that a primary key column was being dropped, even though
that isn't the case.
The problem is that CatalogManager::ApplyAlterSchemaSteps
checked whether a column was a key column by checking by
name against the table's schema before any alter steps were
applied. The fix is to check against the schema with the
previous alter steps applied.
Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
---
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
4 files changed, 21 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/8188/2
--
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: newpatchset
Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Gerrit-Change-Number: 8188
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 2: Code-Review+1
Will defer to Dan.
--
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: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 02 Oct 2017 18:47:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8188 )
Change subject: KUDU-2132 Error dropping columns renamed to old key column names
......................................................................
KUDU-2132 Error dropping columns renamed to old key column names
Suppose a table has two columns 'a' and 'b', and 'a' is the primary
key. Consider the following sequence of alter table steps, done as
part of one alter table request:
1. Rename the key column 'a' to 'c'.
2. Rename 'b' to 'a', the previous name of the key column.
3. Drop 'a'.
This is a valid sequence, but previously we were rejecting it,
saying that a primary key column was being dropped, even though
that isn't the case.
The problem is that CatalogManager::ApplyAlterSchemaSteps
checked whether a column was a key column by checking by
name against the table's schema before any alter steps were
applied. The fix is to check against the schema with the
previous alter steps applied.
Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Reviewed-on: http://gerrit.cloudera.org:8080/8188
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
4 files changed, 21 insertions(+), 1 deletion(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, but someone else must approve
Dan Burkert: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Gerrit-Change-Number: 8188
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 2: Code-Review+2
--
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: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 02 Oct 2017 18:53:58 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley 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)
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 con
Done
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?
Done
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 cur
That's right.
--
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 <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 02 Oct 2017 16:47:50 +0000
Gerrit-HasComments: Yes