You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org> on 2023/01/03 01:17:41 UTC
[kudu-CR] KUDU-1945: Support non unique primary key for Java client
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 <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Jan 2023 01:17:41 +0000
Gerrit-HasComments: Yes