Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19384 )
Change subject: KUDU-1945: Support non unique primary key for Java client ...................................................................... Patch Set 6: (19 comments) Thanks for review. http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java: http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@181 PS6, Line 181: auto incrementing > nit for here and elsewhere: is it possible to get into some consistent nami Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@360 PS6, Line 360: is part > nit: is a part Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@400 PS6, Line 400: auto incrementing column > nit: an auto incrementing column Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/Schema.java File java/kudu-client/src/main/java/org/apache/kudu/Schema.java: http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/Schema.java@44 PS6, Line 44: primary > primary key Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/Schema.java@343 PS6, Line 343: Get column name of auto incrementing column > nit: Get the name of the auto-incrementing column Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/Schema.java@351 PS6, Line 351: Get type of auto incrementing column > nit: Get the type of the auto-incrementing column Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@655 PS6, Line 655: // Create a copy of mutable list and insert auto incrementing column as last key : // columns : List<ColumnSchema> colSchemas = new ArrayList<>(schema.getColumns()); : Preconditions.checkNotNull(colSchemas); > I thought auto-incrementing column comes after columns marked as key column Yes, auto-incrementing column is added after all columns which are marked as key columns. Updated comments. http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@373 PS6, Line 373: auto_incrementing > nit: auto-incrementing Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java: http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@157 PS6, Line 157: public static ColumnSchema pbToColumnSchema(Common.ColumnSchemaPB pb, : boolean hasAutoIncrementing) > Why to introduce this 'hasAutoIncementing' parameter when Common.ColumnSche We need to mark all of key columns as non unique key columns, including the key columns which are not auto-incrementing columns if the table has auto_incrementing column. Rename second parameter as isUniqueKey. http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java File java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java: http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java@79 PS6, Line 79: assertNotEquals(stringCol1, isNonUniqueKey); > In this comparison, 'test' default value is mixed in. Does it make sense t Change test code to compare with unique key http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java@86 PS6, Line 86: assertNotEquals(stringCol1, isAutoIncrementing); > ditto Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java@88 PS6, Line 88: // Different by type > In addition, does it make sense to add 'same' scenarios for auto-incrementi Done http://gerrit.cloudera.org:8080/#/c/19384/6/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/19384/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@805 PS6, Line 805: primary key > nit: the primary key Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@817 PS6, Line 817: auto_incrementing > nit: the auto-incrementing column Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@828 PS6, Line 828: auto_incrementing > auto-incrementing Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@838 PS6, Line 838: auto_incrementing > nit: auto-incrementing Done http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java: http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2582 PS6, Line 2582: } catch (IllegalArgumentException e) { : fail(e.getMessage()); : } > What this try/catch block is for? Would be great to add a comment. remove try/catch http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2612 PS6, Line 2612: } catch (IllegalArgumentException e) { : fail(e.getMessage()); : } > What this try/catch block is for? Would be great to add a comment. remove try/catch http://gerrit.cloudera.org:8080/#/c/19384/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2631 PS6, Line 2631: } > Could you also add scenarios to try creating: added new test cases -- To view, visit http://gerrit.cloudera.org:8080/19384 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7e2501d6b3d66f6466959e4f3f1ed0f5e08dfe5c Gerrit-Change-Number: 19384 Gerrit-PatchSet: 6 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 03 Jan 2023 01:17:41 +0000 Gerrit-HasComments: Yes
