Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20083 )
Change subject: [client] KUDU-1945 Add UPSERT support ...................................................................... Patch Set 7: (12 comments) http://gerrit.cloudera.org:8080/#/c/20083/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java: http://gerrit.cloudera.org:8080/#/c/20083/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@45 PS7, Line 45: import org.checkerframework.checker.units.qual.C; > What is it and why is it needed here? Sorry, looks like my IDE added it when I was playing around. We don't need it. Removed it. http://gerrit.cloudera.org:8080/#/c/20083/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@666 PS7, Line 666: Operation.ChangeType.INSERT > What about INSERT_IGNORE? I originally thought INSERT_IGNORE should not report all the errors but re-read it and added the check for it here. http://gerrit.cloudera.org:8080/#/c/20083/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: http://gerrit.cloudera.org:8080/#/c/20083/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@a908 PS7, Line 908: > Since these negative sub-cases are removed, does it makes sense to add corr Done http://gerrit.cloudera.org:8080/#/c/20083/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@942 PS7, Line 942: // Verify that auto-incrementing column value cannot be specified in an INSERT operation. > Consider adding similar case for INSERT_IGNORE as well. Done http://gerrit.cloudera.org:8080/#/c/20083/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@951 PS7, Line 951: } : catch (KuduException ex) { > nit: join these lines Done http://gerrit.cloudera.org:8080/#/c/20083/7/python/kudu/tests/test_client.py File python/kudu/tests/test_client.py: http://gerrit.cloudera.org:8080/#/c/20083/7/python/kudu/tests/test_client.py@489 PS7, Line 489: op = table.new_upsert() > UPSERT_IGNORE is not added to the Python client yet. (https://github.com/ap Done http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/client/client-test.cc@a10105 PS7, Line 10105: > Does it make sense to add coverage for UPSERT and UPSERT_IGNORE operations Done http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/client/session-internal.cc@356 PS7, Line 356: Status CheckForAutoIncrementingColumn(const KuduWriteOperation& op) { > nit: also consider adding DCHECK() on the op type to make sure this functio Since type() is a protected member of KuduWriteOperation it is not accessible here, not sure we need it anymore considering my next patchset has a more clear function name for inserts and noninserts. http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/client/session-internal.cc@358 PS7, Line 358: IllegalState > Sorry, I didn't notice this in the original review for the prior version of Done http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/client/session-internal.cc@402 PS7, Line 402: RETURN_NOT_OK_ADD_ERROR(CheckForNonNullableColumns, op, error_collector_); > Does it make sense to check that auto-incrementing column is set, if presen There is no client side verification for that. We rely on server side verification for checking the entire primary key presence. I added the check here. Added validation for INSERT_IGNORE as well. http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/common/schema.cc@313 PS7, Line 313: if (col.name() == Schema::GetAutoIncrementingColumnName() && !col.is_auto_incrementing()) { > nit: maybe, revert this change since nothing changed here actually? Introduced the change here in this patch. http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/20083/7/src/kudu/integration-tests/auto_incrementing-itest.cc@247 PS7, Line 247: BasicUpserts > What about BasicUpdates? There is no change of functionality of Updates on the server side hence did not add any tests. But I don't see any harm in adding. -- To view, visit http://gerrit.cloudera.org:8080/20083 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27a95e3a6b1d1b584cad849978313b3c8222cd3d Gerrit-Change-Number: 20083 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Comment-Date: Fri, 23 Jun 2023 23:43:32 +0000 Gerrit-HasComments: Yes
