Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9240 )
Change subject: Add back KuduColumnSchema DataTypeToString ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/9240/2/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/9240/2/src/kudu/client/schema.h@204 PS2, Line 204: static std::string DataTypeToString(DataType type, KuduColumnTypeAttributes type_attributes); KuduColumnTypeAttributes doesn't have a move or copy constructor, so can this even be called? Not sure how that works. http://gerrit.cloudera.org:8080/#/c/9240/2/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/9240/2/src/kudu/client/schema.cc@568 PS2, Line 568: return DataType_Name(ToInternalDataType(type, type_attributes)); Isn't this going to return 'Decimal32'/'Decimal64'/'Decimal128'? I don't think we should expose that - the underlying storage size is entirely an implementation detail. Returning something like 'Decimal(2, 2)' would be better. Also, tests would be nice. -- To view, visit http://gerrit.cloudera.org:8080/9240 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bf6879f5ab623366a962c6a3d51efc49978a53c Gerrit-Change-Number: 9240 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 07 Feb 2018 23:12:22 +0000 Gerrit-HasComments: Yes
