Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8830 )
Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) ...................................................................... Patch Set 6: (4 comments) I only looked at the public API changes. http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@64 PS6, Line 64: public: Nit: indentation http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@70 PS6, Line 70: explicit KuduColumnTypeAttributes(int32_t precision, int32_t scale) The 'explicit' keyword is only needed to avoid implicit conversions with single-arg constructors, no? http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@75 PS6, Line 75: const int32_t precision() const { These accessors are returning an int32_t 'copy', so what value does the 'const' keyword add (as in 'const int32_t ...')? http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@84 PS6, Line 84: private: Do you anticipate KuduColumnTypeAttributes evolving for other use cases in the future? If so, you should PIMPL the class: define a private Data nested class, store precision/scale within it, and have the public class merely store an allocated pointer to the nested class. -- To view, visit http://gerrit.cloudera.org:8080/8830 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 03 Jan 2018 19:18:24 +0000 Gerrit-HasComments: Yes
