helifu 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 prev Done 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. Done 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 It will crash in the old code if we pass in a null pointer, so i think it is necessary to prevent it. And if we can't return nullptr, it's acceptable to return this. So, when data_->default_val is initialized (not boost::none), it should not be null any more. 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 Wow, a big mistake here!! Yes, my intention is to 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: Done 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 No, if we had prevented the null pointer from being passed in, there would have been no need to check s.spec->data_->default_val.value() any more. In addition, in the old code an null pointer can still make a crash even we check s.spec->data_->default_val here. -- 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-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Sat, 30 Mar 2019 15:33:59 +0000 Gerrit-HasComments: Yes
