Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20083 )
Change subject: [client] KUDU-1945 Add UPSERT support ...................................................................... Patch Set 7: (13 comments) http://gerrit.cloudera.org:8080/#/c/20083/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20083/7//COMMIT_MSG@14 PS7, Line 14: Additionally added nit: 'Additionally added ...' --> 'Also, added ...' 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? 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? 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 corresponding positive sub-cases for UPSERT and UPSERT_IGNORE? 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. 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 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() Add a test sub-case for UPSERT_IGNORE as well? 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 that should succeed with this patch? 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 function isn't called mistakenly with with other operation types. 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 this file, but shouldn't it rather be Status::InvalidArgument()? 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 present, for UPDATE/UPDATE_IGNORE, UPSERT/UPSERT_IGNORE? Or that client-side validation is performed elsewhere? 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? 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? -- 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 <[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-Comment-Date: Fri, 23 Jun 2023 06:48:05 +0000 Gerrit-HasComments: Yes
