Ram Mettu has posted comments on this change. Change subject: KUDU-1890 Allow renaming of primary key column ......................................................................
Patch Set 9: (5 comments) Addressed code review comments http://gerrit.cloudera.org:8080/#/c/6078/8/src/kudu/common/schema-test.cc File src/kudu/common/schema-test.cc: Line 120: } > can you add a test that verifies that the nullability has not changed? ie i Cannot add nullability on the key column, it asserts when creating the Schema object http://gerrit.cloudera.org:8080/#/c/6078/8/src/kudu/common/schema.h File src/kudu/common/schema.h: PS8, Line 214: ype_info()->type(); > instead of two bools, how about a bitset of flags? eg COMPARE_DEFAULTS | CO Implemented with bitset parameter, please take a look at the latest changes Line 217: bool Equals(const ColumnSchema &other, > warning: don't use else after return [readability-else-after-return] Done PS8, Line 215: } : : bool Equals(const ColumnSchema &other, : > it seems like these conditions could be simplified a bit, right? you don't Done PS8, Line 657: return true; : } : : // Return true if the key projection schemas have exactly the same set of : // columns and > this is mostly repeated code from the above KeyEquals. if using flags as su Removed the duplicated code, but kept the KeyTypeEquals() so it is easy for callers not have to worry about ColumnSchema::COMPARE* flags -- To view, visit http://gerrit.cloudera.org:8080/6078 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I28a8c52bdb9ac5a3661f9a07c737f7252466d307 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ram Mettu <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Ram Mettu <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
