Todd Lipcon has posted comments on this change. Change subject: KUDU-1890 Allow renaming of primary key column ......................................................................
Patch Set 8: (4 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 if one column is STRING NOT NULL and the other is STRING (nullable), then it should not be KeyTypeEqual http://gerrit.cloudera.org:8080/#/c/6078/8/src/kudu/common/schema.h File src/kudu/common/schema.h: PS8, Line 214: bool check_defaults, bool check_na instead of two bools, how about a bitset of flags? eg COMPARE_DEFAULTS | COMPARE_NAMES PS8, Line 215: if (check_name && (!EqualsType(other) || this->name_ != other.name_)) : return false; : else if (!EqualsType(other)) : it seems like these conditions could be simplified a bit, right? you don't need the EqualsType part in the top condition anymore. Also, I agree with tidybot PS8, Line 657: if (this->num_key_columns_ != other.num_key_columns_) return false; : for (size_t i = 0; i < this->num_key_columns_; i++) { : if (!this->cols_[i].Equals(other.cols_[i], false, false)) return false; : } : return true; this is mostly repeated code from the above KeyEquals. if using flags as suggested then you can just implement this once and propagate the flags into the Equals method -- 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: 8 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
