You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Marton Greber (Code Review)" <ge...@cloudera.org> on 2022/11/23 17:12:27 UTC
[kudu-CR] Auto-Incrementing Column, C++ client part1
Marton Greber has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19272
Change subject: Auto-Incrementing Column, C++ client part1
......................................................................
Auto-Incrementing Column, C++ client part1
This patch adds the first part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves similarly like PrimaryKey:
- only one column spec can be specified,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys doesn't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
A new bool argument is added to KuduScanner's constructor:
include_auto_incrementing. By default, it is true, scan results will
contain the auto incrementing column. Setting this variable to false in
will add a default projection in the constructor to hide away the auto
incrementing column.
--------------------------to be removed---------------------------------
This patch will be rebased on top of two other patches.
The end git log should look something like:
Auto-Incrementing Column, C++ client part1
<this patch's review link>
KUDU-1945 Auto-Incrementing Column
https://gerrit.cloudera.org/#/c/19097/
KUDU-1945 [client] Add UINT64 support to cpp client
https://gerrit.cloudera.org/#/c/19267/
Part2 would be an additional feature to enable schema creation without
any primary or non-unique primary keys. First we should add the auto
incrementing column. Then the partition columns in KuduTableCreator
could be used to make the compound key together with the auto
incrementing column. However, this requires KuduTableCreator to modify
the schema. As of now neither KuduTableCreator nor KuduTable have the
schema as mutable object. I'm going to submit a WIP patch for this soon,
and we can discuss this further.
------------------------------------------------------------------------
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
17 files changed, 397 insertions(+), 89 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/1
--
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: newchange
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 1
Gerrit-Owner: Marton Greber <gr...@gmail.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 24: Verified+1
unrelated test failures in TxnCommitITest.TestConcurrentAbortsAndCommits
--
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: 24
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 04:56:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 21:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc@1542
PS20, Line 1542: return spec;
: }
:
: Kud
> Not sure if we should return "spec" or null? Where "spec" is released in ca
Refactored a checks, such that I moved all the verification logic into: KuduTableAlterer::Data::ToRequest(AlterTableRequestPB* req)
This way this problem doesnt exist.
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc@1554
PS20, Line 1554: Data::Step s = { AlterTableRequestPB::DROP_COLUMN,
> Not sure if we should disable altering all properties of auto-incrementing
Yepp, as explained above, this checks are also moved into KuduTableAlterer::Data::ToRequest(AlterTableRequestPB* req)
where we can check individually.
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/schema.cc@566
PS20, Line 566: Data() : key_cols_unique(true) {
: }
> nit: don't need to split as two lines, one extra space after ":"
Done
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc@373
PS20, Line 373: if (op.row().schema()->has_auto_incrementing()) {
: return Status::IllegalState(
> nit: call op.row().schema()->has_auto_incrementing() instead
Done
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc@376
PS20, Line 376: op.ToString()));
> nit: log the WriteOperation type in the message
This function is used down below, in ValidateWriteOperation, where it is used like:
RETURN_NOT_OK_ADD_ERROR(CheckForAutoIncrementingColumn, op, error_collector_);
This contains the write op. Here is a resulting example message, in case of an UPSERT:
"Illegal state: write operation is not supported on tables with non-unique primary key: UPSERT int32 key=1"
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc@376
PS20, Line 376:
> nit: Should we change to "table with auto-incrementing column"?
My take is the following:
Users interact with non-unique key in schema builder for example.
We are not expecting them to know much about the auto incrementing column.
Thats why I think it's better to give message using non-uniqe primary key in it.
(this they can change, the auto incrementing column not)
Let me know if it would be better to change this.
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/common/schema.h@1048
PS20, Line 1048: auto_incrementi
> nit: it's better to name variable in the same style as other variables in t
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: 21
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jan 2023 15:09:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] Auto-Incrementing Column, C++ client part1
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: Auto-Incrementing Column, C++ client part1
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc
File src/kudu/client/scan_configuration.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@66
PS1, Line 66: if (table->schema().schema_->has_auto_incrementing() && !include_auto_incrementing) {
: auto ids = table->schema().schema_->column_ids();
: vector<int> projection_indexes;
: for (auto id : ids) {
: int idx = table->schema().schema_->find_column_by_id(id);
: if (idx == table->schema().schema_->auto_incrementing_col_idx()) {
: continue;
: }
: projection_indexes.push_back(idx);
: }
: #pragma GCC diagnostic push
: #pragma GCC diagnostic ignored "-Wunused-result"
: this->SetProjectedColumnIndexes(projection_indexes);
: #pragma GCC diagnostic pop
: }
: }
nit: indent
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.cc@582
PS1, Line 582: std::optional<vector<string>> non_unique_key_col_names;
We don't need two vectors if we combine SetPrimaryKey() and SetNonUniquePrimaryKey(). A boolean variable is_non_unique can tell if the primary key is unique or not. This will simplify KuduSchemaBuilder::Build().
--
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: 1
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Nov 2022 08:12:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 10:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/client-unittest.cc
File src/kudu/client/client-unittest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/client-unittest.cc@188
PS9, Line 188:
> Do you mind adding one extra scenario:
Small clarification: there is no UniquePrimaryKey, we only have PrimaryKey and NonUniquePrimaryKey.
There is a case to cover the above mentioned scenario:
TestSchemaBuilder_PrimaryKeyOnColumnAndSetNonUniquePrimaryKey
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/client-unittest.cc@204
PS9, Line 204: ASSERT_OK(b.Build(&s));
> nit here and below for newly added scenarios: there is ASSERT_OK() macro wh
Done
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/client-unittest.cc@361
PS9, Line 361: Status s = RetryFunc(deadline, "retrying test func", "timed out",
: [&](const MonoTime& deadline, bool* retry) {
: return TestFunc(deadline, retry, &counter);
:
> Why is this change?
It was a mistake. I removed this change.
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/client-unittest.cc@386
PS9, Line 386: ASSERT_STR_CONTAINS(s.ToString(),
> why is this change?
It was a mistake. I removed this change.
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/client-unittest.cc@440
PS9, Line 440: U
> nit: could be just $0, so one less argument for Substitute
Done
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/client-unittest.cc@537
PS9, Line 537: EXPECT_EQ(
: Substitute("(\n"
: " $0:key INT32 NOT NULL,\n"
: " PRIMARY KEY (key)\n"
: ")",
: schema.column_id(0)),
: schema.ToString());
: EXPECT_EQ("(\n"
: " key INT32 NOT NULL,\n"
: " PRIMARY KEY (key)\n"
: ")",
: kudu_schema.ToStri
> Why is this change?
It was a mistake. I removed this change.
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/client-unittest.cc@566
PS9, Line 566: KuduColumnSchema a32_dflt("a", KuduColumnSchema::INT32, /*is_nullable=*/false,
: /*is_immutable=*/false, /*is_auto_incrementing=*/false,
: /*default_value=*/&kDefaultOf7);
: ASSERT_NE(a32, a32_dflt);
: }
:
> Why is this change?
It was a mistake. I removed this change.
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/scan_token-test.cc@571
PS9, Line 571:
> nit: Why this customization is necessary? If it's crucial, add a comment t
Actually these are not necessary, removed them.
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/scan_token-test.cc@572
PS9, Line 572: // Insert rows
> ditto
Done
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/scan_token-test.cc@595
PS9, Line 595: // Projection including auto incrementing column
> Does it make sense to check that "col" is present?
I think so, I added checks in all three cases (see below).
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/scan_token-test.cc@614
PS9, Line 614: vector<KuduScanToken*> tokens;
> ditto
Done
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/scan_token-test.cc@633
PS9, Line 633: {
> ditto
Done
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@584
PS9, Line 584:
> nit: Kudu uses convention to pass 'out' parameters using pointers, not refe
Thank you for noticing this! I changed it to a pointer.
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/schema.cc@586
PS9, Line 586:
> Why set NotNull?
Down I use "KuduColumnSpec::ToColumnSchema(KuduColumnSchema* col)". I need to set NotNull() as the default value is true:
(https://github.com/apache/kudu/blob/master/src/kudu/client/schema.cc#L468)
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/client/schema.cc@1031
PS9, Line 1031: indexes->resize(num_key_columns());
> This signature looks a bit odd to me: if returning a copy, why to enforce i
Done
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/common/partial_row.cc@900
PS9, Line 900: DCHECK
> nit: there are DCHECK_LE/DCHECK_GE for this, e.g. it might be
Done
http://gerrit.cloudera.org:8080/#/c/19272/9/src/kudu/common/partial_row.cc@901
PS9, Line 901: DCHECK(schema_->has_auto_incrementing());
> Does this mean it's not allowed to call this method for a partial row of a
Yes, this should be invoked on partial rows which have non-unique keys. (the schema contains the auto incrementing column)
In "KuduSession::Data::ValidateWriteOperation()" (https://gerrit.cloudera.org/#/c/19272/10/src/kudu/client/session-internal.cc) you can see the newly added if statement to distinguish between these two cases.
--
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 <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Jan 2023 17:59:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] Auto-Incrementing Column, C++ client part1
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: Auto-Incrementing Column, C++ client part1
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@7
PS1, Line 7: Auto
Add the Jira to keep track of the patches.
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@17
PS1, Line 17: doesn't
don't
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@25
PS1, Line 25: in
delete
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema-internal.h
File src/kudu/client/schema-internal.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema-internal.h@89
PS1, Line 89: non_unique_primary_key;
> non_unique_primary_key have conflict with primary_key. How about change non
+1
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@499
PS1, Line 499: uqnique
nit: unique
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@582
PS1, Line 582: increment
nit: Let's call it auto-incrementing everywhere to keep it uniform.
--
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: 1
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Nov 2022 17:30:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 6: Code-Review+1
--
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 <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Dec 2022 05:31:56 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 7: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/7/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/7/src/kudu/client/schema.cc@689
PS7, Line 689: if (!data_->key_cols_unique)
nit: change to else
--
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: 7
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Dec 2022 18:02:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/7/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/7/src/kudu/client/schema.cc@689
PS7, Line 689: return Status::InvalidArgum
> nit: change to else
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: 8
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 17:24:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 19:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@798
PS19, Line 798: Utility function to return the actual name of the auto incrementing column
> Yep, that makes sense. Do we want to restrict DDL operations with the auto
Yes, there will be a followup patch on the server side to restrict alter table operations involving auto-incrementing column. For the C++ client side changes, I think Marton was planning to include those changes in the followup changelist along with the server side bits.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@589
PS19, Line 589: std::string Name;
: KuduColumnSchema::DataType Type;
> style nit: the name of these fields should start with non-capital letters
+1
--
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: 19
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 04:30:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 19: Code-Review+1
(10 comments)
Just a few nits, overall looks good to me.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc@338
PS19, Line 338: const KuduScanToken*
style nit: consider using const reference here instead -- in most cases in Kudu codebase, constant parameters are passed by reference, not by pointer
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc@341
PS19, Line 341: shared_ptr<KuduClient> client;
: CHECK_OK(cluster_->CreateClient(nullptr, &client));
Why not to use already existing 'client_' member field here? Would be great to add a short blurb to explain the reasoning behind.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@798
PS19, Line 798: Utility function to return the actual name of the auto incrementing column
I guess this is only for the time when a table with implicitly added column is created? In other words, that's the default name for the auto-incrementing column, IIUC. We don't have any restriction in place to prohibit changing the name of the auto-incrementing column later on using AlterTable, right?
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@867
PS19, Line 867: auto_incrementing_col_name
style nit: this should either follow the naming convention for regular field (like 'schema_' below) and end with underscore or the convention of naming static constant/constexpr static fields, where it would be something like kAutoIncColName or something similar.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@575
PS19, Line 575: vector<KuduColumnSchema>&
style nit: in most of the Kudu code, the "out" and "inout" parameters are usually passed as a pointer
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@589
PS19, Line 589: std::string Name;
: KuduColumnSchema::DataType Type;
style nit: the name of these fields should start with non-capital letters
Shouldn't they be static constexpr ones?
Also, make this private section explicit (i.e. add the 'private:' tag) and move it to be after all the methods and fields in the 'public:' section.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@594
PS19, Line 594:
style nit: add two extra spaces before the column
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@609
PS19, Line 609: bool key_cols_unique;
Should this field be initialized in the constructor?
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@682
PS19, Line 682:
nit: remove this extra line?
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/common/schema.cc@a293
PS19, Line 293:
:
:
:
:
:
:
:
:
:
:
:
Does it make sense to keep the logic to enforce the invariants for auto-incrementing column in the form of corresponding DCHECK() macros to spot programming mistakes?
--
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: 19
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 00:55:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Dec 2022 19:29:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 22:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19272/21/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/21/src/kudu/client/schema.cc@342
PS21, Line 342: KuduColumnSpec* KuduColumnSpec::NonUniquePrimaryKe
> We may remove this API so that user is not allowed to add auto-incrementing
Done
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc@376
PS20, Line 376:
> As discussed in the meeting, server side will also disable upsert for table
Yep, I'm convinced. I changed accordingly.
http://gerrit.cloudera.org:8080/#/c/19272/21/src/kudu/client/table_alterer-internal.cc
File src/kudu/client/table_alterer-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/21/src/kudu/client/table_alterer-internal.cc@116
PS21, Line 116: turn Status::AlreadyPresent("can't add reserved name", s.spec->data_->name);
: }
> Should we not allow user to add an auto-incrementing column in Alter Table
This is a good catch!
Yes we don't want this.
Changed this, to only check the name.
--
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: 22
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jan 2023 11:50:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] Auto-Incrementing Column, C++ client part1
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: Auto-Incrementing Column, C++ client part1
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc@198
PS1, Line 198: const std::string ColumnSchema::auto_incrementing_id = "auto_incrementing_id";
> I guess it makes sense to create a method for that: that's more flexible if
This should be property of KuduSchema object.
--
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: 1
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Nov 2022 21:19:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#15).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/15
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 15
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#10).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 195 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/10
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 10
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 24: Code-Review+1
--
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: 24
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 08:26:03 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#22).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Some technical notes:
- The name of the auto incrementing column is hardcoded into the Schema
class. This is a reserved column name, users can't create columns with
it. On the client facing side, this reserved string is reachable
through: KuduSchema::GetAutoIncrementingColumnName().
- In this initial version there is no support for UPSERT and
UPSERT_IGNORE operations.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/auto_incrementing-itest.cc
16 files changed, 757 insertions(+), 196 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/22
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 22
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#7).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the first part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/tablet/tablet_replica.h
14 files changed, 549 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/7
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 7
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Jan 2023 21:48:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/14/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/14/src/kudu/integration-tests/auto_incrementing-itest.cc@135
PS14, Line 135: I
nit: indent
--
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: 14
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Jan 2023 05:19:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#21).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Some technical notes:
- The name of the auto incrementing column is hardcoded into the Schema
class. This is a reserved column name, users can't create columns with
it. On the client facing side, this reserved string is reachable
through: KuduSchema::GetAutoIncrementingColumnName().
- In this initial version there is no support for UPSERT and
UPSERT_IGNORE operations.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/auto_incrementing-itest.cc
16 files changed, 767 insertions(+), 194 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/21
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 21
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 25: Verified+1
unrelated test failure in AutoAddMasterTest.TestRestartMastersWhileSomeDown (ASAN)
--
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: 25
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 16:32:04 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/scan_token-test.cc@530
PS6, Line 530: }
Add test case without setting projected columns. Verify that auto_incrementing_id is included in the schema for scan token.
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/schema.h@728
PS6, Line 728: Reset
auto_incrementing column is only added to schema when the schema is built by KuduSchemaBuilder::Build(). KuduSchema::Reset() is deprecated API. It does not support non unique primary key and does not add auto_incrementing column. Please indicate this in the comments.
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/schema.cc@682
PS6, Line 682: if (spec->data_->primary_key && data_->key_cols_unique) {
: return Status::InvalidArgument(
: "primary key specified by both SetPrimaryKey() and on a "
: "specific column",
: spec->data_->name);
: }
: if (spec->data_->primary_key && !data_->key_cols_unique) {
change code as:
if (spec->data_->primary_key) {
if (data_->key_cols_unique) {
return ...
} else {
return ...
}
}
--
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 <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Dec 2022 06:33:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#4).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the first part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
12 files changed, 459 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/4
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 4
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/client-unittest.cc
File src/kudu/client/client-unittest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/client-unittest.cc@163
PS2, Line 163: SetNonUniquePrimaryKey
Have another test setting this to primary key
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.cc@656
PS2, Line 656: ,
Delete
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.cc@660
PS2, Line 660: ,
Delete
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.cc@757
PS2, Line 757: +
nit: Here and below missing space around the operators "+" and ","
--
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: 2
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sun, 04 Dec 2022 18:50:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] Auto-Incrementing Column, C++ client part1
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: Auto-Incrementing Column, C++ client part1
......................................................................
Patch Set 1:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@13
PS1, Line 13: similarly like
nit: either 'similar to' or 'like'
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@14
PS1, Line 14: only one column spec can be specified,
Could you add the reasoning why is this restriction?
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc
File src/kudu/client/client-unittest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc@63
PS1, Line 63: no primary key, nor
nit: 'neither ... nor ...'
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc@80
PS1, Line 80: ->PrimaryKey()
: ->NonUniquePrimaryKey();
Mind adding a sub-case with these two calls in different order?
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h@2763
PS1, Line 2763: explicit KuduScanner(KuduTable* table, bool include_auto_incrementing = true);
It seems this change would break backwards ABI compatibility, so this isn't acceptable: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B
The idea is that the newer kudu_client dynamic library should be linkable with applications compiled with older versions of the headers/library.
You can add a extra constructor with a new signature instead, I guess.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.h
File src/kudu/client/scan_configuration.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.h@53
PS1, Line 53: bool include_auto_incrementing = true
The whole idea behind this ScanConfiguration class is to avoid adding many parameters into the constructor, but instead maintain the set of methods that change the configuration for the result.
Please follow the suit and add corresponding method instead.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc
File src/kudu/client/scan_configuration.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@66
PS1, Line 66: table
Since that's the body of the class already, consider using the corresponding members instead of parameters?
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@67
PS1, Line 67: auto ids
Why not to use refrences instead of copying the vector here?
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@70
PS1, Line 70: int idx
Consider adding DCHECK_GE(idx, 0) to spot programming mistakes.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@76
PS1, Line 76: #pragma GCC diagnostic push
: #pragma GCC diagnostic ignored "-Wunused-result"
Instead, wrap the call below into CHECK_OK()
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@78
PS1, Line 78: this->
Why to add 'this->'?
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@585
PS1, Line 585: KuduColumnSpec* AutoIncrementing()
It would be great to actually state what this method does and what it returns.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@643
PS1, Line 643: SetNonUniquePrimaryKey
> How about combining this functions with SetPrimaryKey(), change SetPrimaryK
Wenzhe, that's not a good idea since it would break backwards ABI compatibility (see https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B for details).
Kudu provides backward-compatible ABI with kudu_client library, so it's possible to make applications linked with prior version link with a dynamic library of a newer version.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/partial_row.cc@888
PS1, Line 888: return BitmapIsAllSet(isset_bitmap_, 0, schema_->num_key_columns()-1);
nit: add spaces around '-'
Also, add DCHECK() to make sure schema_->num_key_columns() is greater or equal to 1.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc@198
PS1, Line 198: const std::string ColumnSchema::auto_incrementing_id = "auto_incrementing_id";
> Could we define the column name of auto_incrementing_id as constant and put
I guess it makes sense to create a method for that: that's more flexible if we are ever to change the name for such column in future releases. Should this be a property of a particular Schema object?
--
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: 1
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Nov 2022 22:25:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#2).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the first part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
12 files changed, 459 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/2
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 2
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@755
PS1, Line 755: /// The placeholder for the result.
> need API to to check if the primary key is unique or not, and API to get au
Added: int KuduSchema::GetAutoIncrementingColumnIndex() const
--
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: 2
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Dec 2022 18:28:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 21:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc@1542
PS20, Line 1542: return spec;
: }
:
: Kud
> Refactored a checks, such that I moved all the verification logic into: Kud
Yes, the code change avoid this case.
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc@1554
PS20, Line 1554: Data::Step s = { AlterTableRequestPB::DROP_COLUMN,
> Yepp, as explained above, this checks are also moved into KuduTableAlterer:
That's great.
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc@376
PS20, Line 376:
> My take is the following:
As discussed in the meeting, server side will also disable upsert for table with auto-incrementing column. For Kudu server, it check auto-incrementing, not non unique primary key. We may keep error message consistent.
http://gerrit.cloudera.org:8080/#/c/19272/21/src/kudu/client/table_alterer-internal.cc
File src/kudu/client/table_alterer-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/21/src/kudu/client/table_alterer-internal.cc@116
PS21, Line 116: !s.spec->data_->auto_incrementing) {
: return Status::AlreadyPresent("can't add reserved name", s.spec->data_->name
Should we not allow user to add an auto-incrementing column in Alter Table API at all? If a table is created with unique primary key, so no auto-incrementing column is added to the table. User insert some rows. Then user call Alter Table API to add an auto-incrementing column to the table. Can server handle this case? Is the added auto-incrementing column set as key or not?
--
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: 21
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jan 2023 18:56:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 21:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/21/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/21/src/kudu/client/schema.cc@342
PS21, Line 342: KuduColumnSpec* KuduColumnSpec::AutoIncrementing()
We may remove this API so that user is not allowed to add auto-incrementing column. In KuduAutoIncrementingColumnSpecBuilder class, set data_->auto_incrementing as true directly.
--
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: 21
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jan 2023 19:51:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#11).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/11
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 11
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#19).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/19
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 19
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 19:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/client-unittest.cc
File src/kudu/client/client-unittest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/client-unittest.cc@267
PS19, Line 267:
> Could you also add a scenario where a schema with a INT64 column named "aut
I think Column name 'auto_incrementing_id' should be reserved column name for Kudu engine. To avoid confusion, we should not allow user to add a column named as 'auto_incrementing_id' when creating table or alter table. Java client throw exception if user try to add a column named as 'auto_incrementing_id'.
--
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: 19
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 04:21:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 25:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19272/24/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/24/src/kudu/client/session-internal.cc@375
PS24, Line 375:
> nit: ... this type of write operation ...
Done.
Yes, this is an example message which would be displayed:
"Illegal state: this type of write operation is not supported on table with auto-incrementing column: UPSERT int32 key=1"
http://gerrit.cloudera.org:8080/#/c/19272/24/src/kudu/client/table_alterer-internal.cc
File src/kudu/client/table_alterer-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/24/src/kudu/client/table_alterer-internal.cc@116
PS24, Line 116: add a column with
> nit: ... a column with reserved name
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: 25
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 14:31:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 24:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/23/src/kudu/client/table_alterer-internal.cc
File src/kudu/client/table_alterer-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/23/src/kudu/client/table_alterer-internal.cc@116
PS23, Line 116: InvalidArgumen
> nit: change to InvalidArgument
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: 24
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jan 2023 21:00:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 24: Code-Review+1
--
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: 24
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jan 2023 21:17:20 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 25: Code-Review+2
--
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: 25
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 14:36:38 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#20).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Some technical notes:
- The name of the auto incrementing column is hardcoded into the Schema
class. This is a reserved column name, users can't create columns with
it. On the client facing side, this reserved string is reachable
through: KuduSchema::GetAutoIncrementingColumnName().
- In this initial version there is no support for UPSERT and
UPSERT_IGNORE operations.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/auto_incrementing-itest.cc
16 files changed, 698 insertions(+), 194 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/20
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 20
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 19:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/client-unittest.cc
File src/kudu/client/client-unittest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/client-unittest.cc@267
PS19, Line 267:
Could you also add a scenario where a schema with a INT64 column named "auto_incrementing_id" is created and added into the list of non-unique primary key columns? What's the expected behavior of the client library in that case?
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@798
PS19, Line 798: Utility function to return the actual name of the auto incrementing column
> Java client don't allow to use Alter Table API to rename or drop the auto-i
Yep, that makes sense. Do we want to restrict DDL operations with the auto-incrementing column at the server side as well? Or we rather want to have this only as the client-side restriction since there isn't any inconsistencies from the server-side point of view if renaming and the auto-incrementing column?
I guess we should at least restrict dropping the auto-incrementing column at the server side since the column is a part of the primary key, and Kudu doesn't properly handle update of the primary key columns as well as columns participating in partition schema.
--
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: 19
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 02:16:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 22:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19272/22/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19272/22/src/kudu/client/client-test.cc@9993
PS22, Line 9993: incrementin
nit: incrementing
Same elsewhere
http://gerrit.cloudera.org:8080/#/c/19272/22/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/22/src/kudu/client/session-internal.cc@398
PS22, Line 398:
nit: extra space
--
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: 22
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jan 2023 15:22:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 23:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19272/22/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19272/22/src/kudu/client/client-test.cc@9993
PS22, Line 9993: incrementin
> nit: incrementing
Done
http://gerrit.cloudera.org:8080/#/c/19272/22/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/22/src/kudu/client/session-internal.cc@398
PS22, Line 398: n
> nit: extra space
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: 23
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jan 2023 17:09:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 23: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/23/src/kudu/client/table_alterer-internal.cc
File src/kudu/client/table_alterer-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/23/src/kudu/client/table_alterer-internal.cc@116
PS23, Line 116: AlreadyPresent
nit: change to InvalidArgument
--
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: 23
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jan 2023 20:44:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 8: Code-Review+1
--
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: 8
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 17:37:12 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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: Set the column to be a non-unique primary key of the table.
> Please add comments to describe the behavior.
Also add comments for the behavior for function PrimaryKey().
--
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 <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Dec 2022 02:18:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] Auto-Incrementing Column, C++ client part1
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: Auto-Incrementing Column, C++ client part1
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h@3226
PS1, Line 3226: DeserializeIntoScanner(
If auto_increment_id column is added in token, the scanner have to read auto_increment_id column.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema-internal.h
File src/kudu/client/schema-internal.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema-internal.h@89
PS1, Line 89: non_unique_primary_key;
non_unique_primary_key have conflict with primary_key. How about change non_unique_primary_key to non_unique_key? For unique primary key, primary_key is set as true, non_unique_key is set as false. For non unique primary key, primary_key and non_unique_key are set as true.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@497
PS1, Line 497: SetNonPrimaryKey
nit: SetNonUniquePrimaryKey()
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@502
PS1, Line 502: NonUniquePrimaryKey
Two sets of functions PrimaryKey() and NonUniquePrimaryKey() are defined for unique keys and non unique keys. We have to check if both functions are called.
How about combining two functions as one, change PrimaryKey() as "KuduColumnSpec* PrimaryKey(bool is_non_unique = false)"?
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@643
PS1, Line 643: SetNonUniquePrimaryKey
How about combining this functions with SetPrimaryKey(), change SetPrimaryKey() as "KuduSchemaBuilder* SetPrimaryKey(const std::vector<std::string>& key_col_names, bool is_non_unique = false)"?
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@755
PS1, Line 755:
need API to to check if the primary key is unique or not, and API to get auto_increment_id index.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc@198
PS1, Line 198: const std::string ColumnSchema::auto_incrementing_id = "auto_incrementing_id";
Could we define the column name of auto_incrementing_id as constant and put it in client.h?
For CTAS, Impala planner create a temporary KuduTable for insertion. Impala has to simulate Kudu engine to add auto_incrementing_id column in the temporary KuduTable for non unique primary key. We need to use same column name.
--
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: 1
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Nov 2022 10:10:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] Auto-Incrementing Column, C++ client part1
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: Auto-Incrementing Column, C++ client part1
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@643
PS1, Line 643: SetNonUniquePrimaryKey
> Wenzhe, that's not a good idea since it would break backwards ABI compatibi
Got it. Thanks for the explanation.
--
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: 1
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Nov 2022 22:45:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#23).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Some technical notes:
- The name of the auto incrementing column is hardcoded into the Schema
class. This is a reserved column name, users can't create columns with
it. On the client facing side, this reserved string is reachable
through: KuduSchema::GetAutoIncrementingColumnName().
- In this initial version there is no support for UPSERT and
UPSERT_IGNORE operations.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/auto_incrementing-itest.cc
16 files changed, 757 insertions(+), 196 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/23
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 23
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#24).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Some technical notes:
- The name of the auto incrementing column is hardcoded into the Schema
class. This is a reserved column name, users can't create columns with
it. On the client facing side, this reserved string is reachable
through: KuduSchema::GetAutoIncrementingColumnName().
- In this initial version there is no support for UPSERT and
UPSERT_IGNORE operations.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/auto_incrementing-itest.cc
16 files changed, 757 insertions(+), 196 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/24
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 24
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 24: Code-Review+2
(5 comments)
Overall looks good to me!
Just a couple of nits in the error messages to make them less surprising, if they are ever read by humans :)
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@798
PS19, Line 798: @note The new row refers to this KuduSchema object, so it must be
> Since this came up during review, I took some time to add restrictions to t
That's great. Thanks a lot!
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@589
PS19, Line 589: }
:
> Done x3.
Yes, that change makes sense to me. And the new place to keep that constant looks like a better option. Thanks!
http://gerrit.cloudera.org:8080/#/c/19272/24/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/24/src/kudu/client/session-internal.cc@375
PS24, Line 375: write operation
nit: ... this type of write operation ...
And op.ToString() should contain the information on the op's type, right?
http://gerrit.cloudera.org:8080/#/c/19272/24/src/kudu/client/table_alterer-internal.cc
File src/kudu/client/table_alterer-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/24/src/kudu/client/table_alterer-internal.cc@116
PS24, Line 116: add reserved name
nit: ... a column with reserved name
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/common/schema.cc@a293
PS19, Line 293:
:
:
:
:
:
:
:
:
:
:
:
> Thinking more about it, yes I think it makes sense.
Cool, thanks!
--
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: 24
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 04:56:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#17).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/17
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 17
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#13).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/13
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 13
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 23:18:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 2:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@7
PS1, Line 7: KUDU
> Add the Jira to keep track of the patches.
Done
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@13
PS1, Line 13: like PrimaryKe
> nit: either 'similar to' or 'like'
Done
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@14
PS1, Line 14: only one column can have the NonUnique
> Could you add the reasoning why is this restriction?
What I ment here is like with PrimaryKey(), only one column can be marked with this ColumnSpec in KuduSchemaBuilder. This is the case with NonUniquePrimaryKey() as well.
Changed the commit message to explain this.
http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@17
PS1, Line 17: set fu
> don't
Done
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc
File src/kudu/client/client-unittest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc@63
PS1, Line 63: neither primary key
> nit: 'neither ... nor ...'
Done
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc@80
PS1, Line 80: b.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()
: ->PrimaryKey()
> Mind adding a sub-case with these two calls in different order?
Done
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h@2763
PS1, Line 2763: explicit KuduScanner(KuduTable* table);
> It seems this change would break backwards ABI compatibility, so this isn't
Done
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h@3226
PS1, Line 3226: eration result status.
> If auto_increment_id column is added in token, the scanner have to read au
This is working. To confirm, I added test see: scan_token-test.cc
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.h
File src/kudu/client/scan_configuration.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.h@53
PS1, Line 53:
> The whole idea behind this ScanConfiguration class is to avoid adding many
Thank you for the explanation, understood. I removed this approach.
Feature-wise, users can keep or discard the auto-incrementing column by using one of the SetProjectedColumn functions, or with scan tokens.
Now understanding ABI requirements, and the weight with adding stuff to the client: do we want to add convenience function to KuduScanner, to handle auto-incrementing column projection? Like adding a function KuduScanner::IncludeAutoIncrementingColumn(bool include=true).
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@497
PS1, Line 497: SetNonUniquePrim
> nit: SetNonUniquePrimaryKey()
Done
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@499
PS1, Line 499: unique
> nit: unique
Done
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@582
PS1, Line 582: increment
> nit: Let's call it auto-incrementing everywhere to keep it uniform.
Done
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@585
PS1, Line 585: // Users are not expected to acces
> It would be great to actually state what this method does and what it retur
Added detailed comments here.
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/partial_row.cc@888
PS1, Line 888: DCHECK(schema_->num_key_columns() >= 1);
> nit: add spaces around '-'
Done
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc@198
PS1, Line 198: Schema::Schema(const Schema& other)
> This should be property of KuduSchema object.
Moved this constant to: const string KuduSchema::auto_incrementing_id() . I added this as static function.
--
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: 2
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Dec 2022 18:09:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#8).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the first part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/tablet/tablet_replica.h
14 files changed, 548 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/8
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 8
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 7:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/scan_token-test.cc@530
PS6, Line 530: // Projection excluding auto incrementing column
> Add test case without setting projected columns. Verify that auto_increment
Done
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/schema.h@728
PS6, Line 728: ram [
> auto_incrementing column is only added to schema when the schema is built b
Done
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/6/src/kudu/client/schema.cc@682
PS6, Line 682: if (spec->data_->primary_key) {
: if (data_->key_cols_unique) {
: return Status::InvalidArgument(
: "primary key specified by both SetPrimaryKey() and on a "
: "specific column",
: spec->data_->name);
: }
> change code as:
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: 7
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Dec 2022 13:38:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#16).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/16
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 16
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Some technical notes:
- The name of the auto incrementing column is hardcoded into the Schema
class. This is a reserved column name, users can't create columns with
it. On the client facing side, this reserved string is reachable
through: KuduSchema::GetAutoIncrementingColumnName().
- In this initial version there is no support for UPSERT and
UPSERT_IGNORE operations.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Reviewed-on: http://gerrit.cloudera.org:8080/19272
Reviewed-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Abhishek Chennaka <ac...@cloudera.com>
Tested-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/auto_incrementing-itest.cc
16 files changed, 759 insertions(+), 196 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
Abhishek Chennaka: Looks good to me, but someone else must approve
--
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: merged
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 26
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 25
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 20:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc@1542
PS20, Line 1542: if (name == Schema::GetAutoIncrementingColumnName()) {
: data_->status_ = Status::InvalidArgument(Substitute(
: "can't add reserved name $0 column", Schema::GetAutoIncrementingColumnName()));
: }
Not sure if we should return "spec" or null? Where "spec" is released in case of error?
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/client.cc@1554
PS20, Line 1554: if (name == Schema::GetAutoIncrementingColumnName()) {
Not sure if we should disable altering all properties of auto-incrementing column? Java client don't allow user to alter column name, type, immutable, default value, etc. But allow user to alter compression algorithm, block size, encoding, comment.
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/schema.cc@566
PS20, Line 566: Data()
: : key_cols_unique(true)
nit: don't need to split as two lines, one extra space after ":"
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc@373
PS20, Line 373: const auto auto_incrementing_col_idx = op.row().schema()->auto_incrementing_col_idx();
: if (auto_incrementing_col_idx != Schema::kColumnNotFound)
nit: call op.row().schema()->has_auto_incrementing() instead
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc@376
PS20, Line 376: with non-unique primary key
nit: Should we change to "table with auto-incrementing column"?
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/client/session-internal.cc@376
PS20, Line 376: write operation
nit: log the WriteOperation type in the message
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/20/src/kudu/common/schema.h@1048
PS20, Line 1048: kAutoIncColName
nit: it's better to name variable in the same style as other variables in this class
--
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: 20
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jan 2023 23:59:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#12).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/12
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 12
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 13:
(2 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@1031
PS9, Line 1031: indexes->resize(num_key_columns());
> I guess Alexey's suggestion is to change the return type as "const string&"
Ah okay, sorry I misunderstood it.
Anyway the linter was complaining that std::string shouldn't be used for global/static string constants. C style is preferred. I changed it to 'const char* const'.
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: return AddColumn(ColumnSchema(name, type, is_nullable, is_immutable,
: is_auto_incrementing, read_default, write_default), false);
: }
:
: Status SchemaBuilder::RemoveColumn(const string& name) {
: unordered_set<string>::const_iterator it_names = col_names_.find(name);
: if (it_names == col_names_.end()) {
: return Status::NotFound("The specified column does not exist", name);
: }
:
: col_names_.erase(it_names);
: f
> Do we still need these checking?
Good catch, I removed these as well.
--
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: 13
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Jan 2023 17:01:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#3).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the first part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
12 files changed, 459 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/3
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 3
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#6).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the first part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/tablet/tablet_replica.h
14 files changed, 528 insertions(+), 127 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/6
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 6
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 3:
(3 comments)
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: for (const int column_idx : message.projected_column_idx()) {
: column_indexes.push_back(column_idx);
: }
> Right now I would lean towards including the auto incrementing column as th
That's fine for Impala integration. Impala always create Kudu scan token with projected columns. For update/delete, the projected columns of scan token consist all key columns, including auto_incrementing_id. But for statements like "create table as select *", auto_incrementing_id has to be omitted from projected columns.
Please add a comment with TODO here.
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: Set the column to be a non-unique primary key of the table.
> I changed the implementation to what you suggested. It really simplifies th
Please add comments to describe the behavior.
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.h@638
PS2, Line 638: This may be used to specify a
> Done
Please add comments to describe the behavior.
--
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: 3
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 18:14:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/client-unittest.cc
File src/kudu/client/client-unittest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/client-unittest.cc@163
PS2, Line 163: "SetNonUniqueP
> Have another test setting this to primary key
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: for (const int column_idx : message.projected_column_idx()) {
: column_indexes.push_back(column_idx);
: }
> If projected_column_idx are not set, all of columns including auto_incremen
Right now I would lean towards including the auto incrementing column as the default. Users can then use projection if they don't need the auto incrementing column. I added some notes, which describe this default behaviour in src/kudu/client/schema.h
In a previous comment I also asked how much 'quality of life' changes do we wanna create. (like adding a switches for showing/disabling auto incrementing etc.)
This patch is feature complete in a sense, that by using projections users can toggle the auto incrementing column. We can add more utility functions in a followup patch, if requested.
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: Set the column to be a non-unique primary key of the table.
> In current implementation, an error will be returned if both PrimaryKey() a
I changed the implementation to what you suggested. It really simplifies the builder's build function. I appreciate this suggestion!
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.h@638
PS2, Line 638: This may be used to specify a
> Same comments as KuduColumnSpec::NonUniquePrimaryKey(), a call to SetNonUni
Done
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.h@766
PS2, Line 766: assumption may not hold, so cal
> What's the return value if there is no auto_incrementing_column? -1?
Yes, it's -1.
To make this clear, I updated the return value's description.
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.cc@757
PS2, Line 757:
> nit: Here and below missing space around the operators "+" and ","
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: 3
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 17:21:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 2:
(2 comments)
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: for (const int column_idx : message.projected_column_idx()) {
: column_indexes.push_back(column_idx);
: }
If projected_column_idx are not set, all of columns including auto_increment_id column will be added to scan token. Should we add an API or flag to control if auto_increment_id column will be added to scan token by default? At least we should document this behavior.
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@766
PS2, Line 766: The placeholder for the result.
What's the return value if there is no auto_incrementing_column? -1?
--
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: 2
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 00:03:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#18).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/18
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 18
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#14).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/integration-tests/auto_incrementing-itest.cc
14 files changed, 546 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/14
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 14
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/14/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/14/src/kudu/integration-tests/auto_incrementing-itest.cc@135
PS14, Line 135: IN
> nit: indent
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: 16
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Jan 2023 06:54:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 19:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@798
PS19, Line 798: Utility function to return the actual name of the auto incrementing column
> I guess this is only for the time when a table with implicitly added column
Java client don't allow to use Alter Table API to rename or drop the auto-incrementing column. c++ client should also add such restrictions.
--
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: 19
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 01:46:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
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 20:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/client-unittest.cc
File src/kudu/client/client-unittest.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/client-unittest.cc@267
PS19, Line 267:
> I think Column name 'auto_incrementing_id' should be reserved column name f
In the new revision, I made the auto incrementing column name a reserved name. Users can't use it to name columns. I added tests
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc@338
PS19, Line 338:
> style nit: consider using const reference here instead -- in most cases in
Thanks for the explanation. Done.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc@341
PS19, Line 341: CHECK_OK(token.Serialize(&serialized_token));
: KuduScanner* scanner_ptr;
> Why not to use already existing 'client_' member field here? Would be grea
We can totally use existing 'client_' member field. Removed this unnecessary part.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@798
PS19, Line 798: Utility function to return the actual name of the auto incrementing column
> Yes, there will be a followup patch on the server side to restrict alter ta
Since this came up during review, I took some time to add restrictions to the alter table operations in this patch:
-users can't drop the auto incrementing column,
-users can't rename the auto incrementing column,
-users can't add a new column with the reserved auto incrementing column name.
Moreover added checks to make the auto incrementing column a reserved column name, which can't be used by users to name columns.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@867
PS19, Line 867:
> style nit: this should either follow the naming convention for regular fiel
Done
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@575
PS19, Line 575:
> style nit: in most of the Kudu code, the "out" and "inout" parameters are u
Thanks for explaining! Done.
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@589
PS19, Line 589: class KuduAutoIncrementingColumnSpecBuilder {
: public:
> style nit: the name of these fields should start with non-capital letters
Done x3.
Small update on the GetAutoIncrementingColumnName() function.
Previously it was part of KuduSchema. There were places in common/schema where I needed this method. But to link from client->common introduces circular dependency. So I moved the column name definition into common/schema. Then passed it 'up' to client/schema. (And if I understand the architecture correctly this is the purpose of common and client separation).
Without this change I couldn't make this name constexpr, as the GetAutoIncrementingColumnName() would lie in client/schema which can't use constexpr for the old cpp version compatibility. (at least the client example failed).
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@594
PS19, Line 594: ~K
> style nit: add two extra spaces before the column
Done
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/common/schema.cc@a293
PS19, Line 293:
:
:
:
:
:
:
:
:
:
:
:
> Does it make sense to keep the logic to enforce the invariants for auto-inc
Thinking more about it, yes I think it makes sense.
Users have the SchemaBuilder API, however internally there are various ways, where these requires might not be set, eg creating custom tests. Like:
Schema schema = Schema({
ColumnSchema("c0", INT32),
ColumnSchema(Schema::GetAutoIncrementingColumnName(),
INT64, false,false, true),
},2);
This is from the auto_incrementing-itest.cc
The Schema constructor does not use the builder api, which utilises the AutoIncrementingColumnSpecBuilder, which pins down the properties.
After similar consideration, I added DCHECKs for the checks in this file.
Thank you for noticing this!
--
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: 20
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jan 2023 22:20:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Patch Set 25: Code-Review+1
--
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: 25
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 16:27:02 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#25).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the initial part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Some technical notes:
- The name of the auto incrementing column is hardcoded into the Schema
class. This is a reserved column name, users can't create columns with
it. On the client facing side, this reserved string is reachable
through: KuduSchema::GetAutoIncrementingColumnName().
- In this initial version there is no support for UPSERT and
UPSERT_IGNORE operations.
As suggested in the server side patch [1], a specific builder is added
to construct the column spec for the auto incrementing column. Since
this pins down the properties of the column, I took the liberty to
remove unit tests and checks which verify these properties.
[1] https://gerrit.cloudera.org/#/c/19097/
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/auto_incrementing-itest.cc
16 files changed, 759 insertions(+), 196 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/25
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 25
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19272
to look at the new patch set (#5).
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
KUDU-1945 Auto-Incrementing Column, C++ client
This patch adds the first part of the cpp client side changes to the
auto incrementing column feature.
A new KuduColumnSpec called NonUniquePrimaryKey is added. Semantically
it behaves like PrimaryKey:
- only one column can have the NonUniquePrimaryKey ColumnSpec in a given
KuduSchemaBuilder context,
- if it exists, it must be defined in the first place,
- compound keys are defined through a set function.
Functionally non-unique primary keys don't need to fulfill the
uniqueness constraint. An auto incrementing column is added in the
background automatically once a non-unique primary key is specified. The
non-unique keys and the auto incrementing columns together form the
effective primary key.
Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/tablet/tablet_replica.h
14 files changed, 755 insertions(+), 351 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/19272/5
--
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: newpatchset
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 5
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b
Gerrit-Change-Number: 19272
Gerrit-PatchSet: 24
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Auto-Incrementing Column, C++ client
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
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 2:
(2 comments)
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: Set the column to be a non-unique primary key of the table.
In current implementation, an error will be returned if both PrimaryKey() and NonUniquePrimaryKey() are called. It's better to change the behavior as a call to these two methods override any previous calls to these two methods, like the behaviors of KuduScanner::SetProjectedColumnNames() and KuduScanner::SetProjectedColumnIndexes().
In this way, the column will be key if one of PrimaryKey() and NonUniquePrimaryKey() is called. We could define another variable to distinguish is the key is unique or not. It also simplify the change for KuduSchemaBuilder::Build().
http://gerrit.cloudera.org:8080/#/c/19272/2/src/kudu/client/schema.h@638
PS2, Line 638: Set the non-unique primary key
Same comments as KuduColumnSpec::NonUniquePrimaryKey(), a call to SetNonUniquePrimaryKey() or SetPrimaryKey() override a previous call to SetNonUniquePrimaryKey() or SetPrimaryKey()
--
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: 2
Gerrit-Owner: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sun, 04 Dec 2022 19:02:09 +0000
Gerrit-HasComments: Yes