Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client ...................................................................... Patch Set 10: (5 comments) looks good, some minor comments http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/schema.cc@586 PS9, Line 586: > Down I use "KuduColumnSpec::ToColumnSchema(KuduColumnSchema* col)". I need Got it. http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/schema.cc@1031 PS9, Line 1031: indexes->resize(num_key_columns()); > Done I guess Alexey's suggestion is to change the return type as "const string&". http://gerrit.cloudera.org:8080/#/c/19272/10/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/19272/10/src/kudu/client/schema.cc@596 PS10, Line 596: } nit: indent http://gerrit.cloudera.org:8080/#/c/19272/10/src/kudu/client/schema.cc@606 PS10, Line 606: }; nit: indent http://gerrit.cloudera.org:8080/#/c/19272/10/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/19272/10/src/kudu/common/schema.cc@639 PS10, Line 639: if (is_auto_incrementing) { : if (is_nullable || is_immutable) { : return Status::InvalidArgument("auto-incrementing column cannot be nullable or immutable"); : } : if (read_default != nullptr || write_default != nullptr) { : return Status::InvalidArgument("auto-incrementing column cannot have read/write " : "defaults set"); : } : if (type != kudu::INT64) { : return Status::InvalidArgument("auto-incrementing column should be of type INT64"); : } : } Do we still need these checking? -- To view, visit http://gerrit.cloudera.org:8080/19272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b Gerrit-Change-Number: 19272 Gerrit-PatchSet: 10 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 12 Jan 2023 21:48:14 +0000 Gerrit-HasComments: Yes
