Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18148 )
Change subject: [tools] Kudu table schema in Avro format revisited This is a follow-up patch to 55cab44 addressing the additional comments posted after the change has been cherry picked. ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/kudu-admin-test.cc@2038 PS1, Line 2038: nit: remove extra spaces http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/tool_action_table.cc@252 PS1, Line 252: return Status::OK(); Is this a premature return from the function? http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/tool_action_table.cc@275 PS1, Line 275: Status PopulateAvroSchema(const string& table_name, Overall, I think it would be nice if you add some references here or in the commit message about Avro types. Also, outlining the basic ideas behind the mapping of the Kudu types into supported Avro types could be helpful along with mentioning the minimum version of the Avro specification this mapping is supposed to work with. http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/tool_action_table.cc@288 PS1, Line 288: Schema nit: could be 'const' since it's not being changed in the scope below? http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/tool_action_table.cc@316 PS1, Line 316: case kudu::client::KuduColumnSchema::BINARY: : RETURN_NOT_OK(AddPrimitiveType(schema.column(i), "binary", writer)); : break; : case kudu::client::KuduColumnSchema::VARCHAR: : RETURN_NOT_OK(AddPrimitiveType(schema.column(i), "varchar", writer)); : break; Does Avro have "binary" and "varchar" types to directly map Kudu's BINARY and VARCHAR types to? -- To view, visit http://gerrit.cloudera.org:8080/18148 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0623812402a188e5b24bbde3db7ef0e3b4c618ec Gerrit-Change-Number: 18148 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 14 Jan 2022 01:55:38 +0000 Gerrit-HasComments: Yes
