[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

["Adar Dembo (Code Review)" <ge...@cloudera.org>]

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 <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 03 Jan 2018 19:18:24 +0000
Gerrit-HasComments: Yes