Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client ...................................................................... Patch Set 6: (12 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: > nit: use different column name like 'y' Done http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/client-unittest.cc@113 PS4, Line 113: > nit: use different column name Done http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/scan_token-internal.cc@207 PS2, Line 207: omitted from projected columns. We could provide API to toggle the presence of : // the auto incrementing column. : vec > That's fine for Impala integration. Impala always create Kudu scan token wi Okay, added a TODO here. 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: : /// @attention In current versions of Kudu, these will always be contiguous : /// column index > Impala has to simulate Kudu engine to add auto_incrementing_id column in it This is just general information. The actual position of the auto incrementing column can be obtained through: KuduSchema::GetAutoIncrementingColumnIndex() http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.h@493 PS2, Line 493: ColumnSpec* PrimaryKey(); > Also add comments for the behavior for function PrimaryKey(). Done http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.h@638 PS2, Line 638: @return A KuduColumnSpec objec > Please add comments to describe the behavior. Done 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: SERIAL > Patch for KUDU-1945 (Add UINT64 support to cpp client) was already merged. Done http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@643 PS4, Line 643: > primary key or non unique primary key Done http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@654 PS4, Line 654: specif > change to SERIAL after rebase Done http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@654 PS4, Line 654: > NonUniquePrimaryKey()? Good catch! Actually this isn't needed here. PrimaryKey(), and NonUniquePrimaryKey() set variables which are used in the builder. Here we are in the builder, these two don't make sense to use inside the builder. The information that a new key column is inserted, is handled by incrementing the num_key_cols variable (see down). Remved. http://gerrit.cloudera.org:8080/#/c/19272/4/src/kudu/client/schema.cc@712 PS4, Line 712: if (key_col_indexes[i] != i) { : return Status::InvalidArgument("primary key columns must be listed first in the schema", : data_->key_col_names.value()[i]); : } : } : : num_key_cols = key_col_indexes.size(); : : if (!data_->key_c > We can share the same code in line #653 to #663. Extracted into a method, to reuse it. 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: // Utility code > add DCHECK(schema_->has_auto_incrementing()) Done -- 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: 6 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: Fri, 09 Dec 2022 19:29:35 +0000 Gerrit-HasComments: Yes
