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 <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 12 Jan 2023 17:59:19 +0000 Gerrit-HasComments: Yes
