Alexey Serbin 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: (12 comments) It seems I took a quick look at PS1 some time ago. Will re-visit this week. http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/client/schema.h@64 PS5, Line 64: public: nit: alignment http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@71 PS1, Line 71: : pre I don't think this is needed unless we want to protect against list-initialized constructors. http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@76 PS1, Line 76: ret drop http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@81 PS1, Line 81: ret drop http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@321 PS1, Line 321: /// Add the documented description for the precision parameter. http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@328 PS1, Line 328: /// Add the documented description for the scale parameter. http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto@54 PS5, Line 54: DECIMAL32 = 17; nit: could you add a comment to explain why 15 and 16 are skipped? http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h File src/kudu/common/decimal_value.h: http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h@32 PS5, Line 32: nit: spacing http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h@43 PS5, Line 43: nit: by code guidelines, the indent should be 2 spaces. http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@50 PS1, Line 50: DataType type) const { nit: misaligned line for the second parameter http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@57 PS1, Line 57: true Why true, not false? If there is some specific reason, please add a comment about that. http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@125 PS1, Line 125: return strings::Substitute("$0$1 $2", nit: missed space -- 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: Alexey Serbin <aser...@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:57:40 +0000 Gerrit-HasComments: Yes
