Abhishek Chennaka 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: (11 comments) http://gerrit.cloudera.org:8080/#/c/18148/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18148/1//COMMIT_MSG@7 PS1, Line 7: [tools] Kudu table schema in Avro format revisited > New line after this Done http://gerrit.cloudera.org:8080/#/c/18148/1//COMMIT_MSG@8 PS1, Line 8: This is a follow-up patch to 55cab44 addressing the additional > Leave a blank line between commit header and description Done 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 Done 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@242 PS1, Line 242: JsonWriter& writer > As per our C++ coding guidelines, we use pointer for output parameters. Done http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/tool_action_table.cc@242 PS1, Line 242: Status AddLogicalType(JsonWriter& writer, const string& type, const string& logical_type, > warning: non-const reference parameter 'writer', make it const or use a poi Done 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? No, fixed it now http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/tool_action_table.cc@259 PS1, Line 259: Status AddPrimitiveType(const ColumnSchema& col_schema, const string& type, JsonWriter& writer) { > warning: non-const reference parameter 'writer', make it const or use a poi Done http://gerrit.cloudera.org:8080/#/c/18148/1/src/kudu/tools/tool_action_table.cc@270 PS1, Line 270: writer.String(schema.Stringify(col_schema.read_default_value())); > error: use of undeclared identifier 'schema' [clang-diagnostic-error] Done 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 Done 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? Done 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 a No it doesn't. Thanks for pointing that out. Fixed it now. -- 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: Tue, 18 Jan 2022 02:02:36 +0000 Gerrit-HasComments: Yes
