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 4: (9 comments) Overall looks good to me, some additional minor comments. http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/client-unittest.cc File src/kudu/client/client-unittest.cc: http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/client-unittest.cc@103 PS4, Line 103: x nit: use different column name like 'y' http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/client-unittest.cc@113 PS4, Line 113: x nit: use different column name http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.h@775 PS4, Line 775: The auto incrementing column is : /// always the last key column. However, in future versions this assumption : /// may not hold Impala has to simulate Kudu engine to add auto_incrementing_id column in its internal temporary table for CTAS statement. The temporary table must have the same layout as the table created by Kudu engine. We need an API to tell where auto_incrementing_id will be added. http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@131 PS4, Line 131: UINT64 Patch for KUDU-1945 (Add UINT64 support to cpp client) was already merged. Sync up with upstream repo. http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@643 PS4, Line 643: primary key primary key or non unique primary key http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@654 PS4, Line 654: UINT64 change to SERIAL after rebase http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@654 PS4, Line 654: PrimaryKey NonUniquePrimaryKey()? http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@712 PS4, Line 712: auto c = new KuduColumnSpec(KuduSchema::auto_incrementing_id()); : c->Type(KuduColumnSchema::UINT64)->NotNull()->AutoIncrementing()->PrimaryKey(); : data_->specs.insert(data_->specs.begin() + num_key_cols, c); : : cols.insert(cols.begin() + num_key_cols, KuduColumnSchema()); : RETURN_NOT_OK(data_->specs[num_key_cols]->ToColumnSchema(&cols[num_key_cols])); : : // Make the above inserted auto-incrementing column a key column. : num_key_cols++; We can share the same code in line #653 to #663. http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/common/partial_row.cc@888 PS4, Line 888: DCHECK(schema_->num_key_columns() >= 1); add DCHECK(schema_->has_auto_incrementing()) -- 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: 4 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: Tue, 06 Dec 2022 23:18:46 +0000 Gerrit-HasComments: Yes
