Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12891 )
Change subject: client: convert the has_foo booleans into boost::optional ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/schema-internal.h File src/kudu/client/schema-internal.h: http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/schema-internal.h@80 PS1, Line 80: boost::optional<bool> primary_key; Not sure why this should be optionalized; there was no has_primary_key previously. http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/schema-internal.h@82 PS1, Line 82: boost::optional<bool> remove_default; // For ALTER Likewise, there was no has_remove_default. http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/schema.cc@225 PS1, Line 225: if (value == nullptr) return nullptr; We can't return nullptr; this is a builder. If the intent is to do nothing when value==nullptr, you should return this. http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/schema.cc@388 PS1, Line 388: if (col_delta->remove_default && col_delta->default_value) { : return Status::InvalidArgument("new default set but default also removed", data_->name); : } I think the old placement made sense: see how L393 and L397 potentially set col_delta->default_value and col_delta->remove_default? Alternatively, perhaps L388 should test data_->remove_default and data_->default_value? http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/schema.cc@533 PS1, Line 533: data_->key_col_names.value().at(i)); Don't need bounds checking offered by at(), just do: data_->key_col_names.value()[i] Or if operator precedence is an issue: (data_->key_col_names.value())[i] http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/table_alterer-internal.cc File src/kudu/client/table_alterer-internal.cc: http://gerrit.cloudera.org:8080/#/c/12891/1/src/kudu/client/table_alterer-internal.cc@a105 PS1, Line 105: : The old code did both of these checks. For boost::optional, I think that'd be: !s.spec->data_->default_val && !s.spec->data_->default_val.value() != nullptr && I see you've omitted the second check here and below; do we not need it? -- To view, visit http://gerrit.cloudera.org:8080/12891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4e7319c617bd388730d6e81378b37eb6793cf24 Gerrit-Change-Number: 12891 Gerrit-PatchSet: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 29 Mar 2019 20:00:11 +0000 Gerrit-HasComments: Yes
