Alexey Serbin 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 10:
(5 comments)
It seems there is a build failure:
15:16:19 [ 69%] Building CXX object
src/kudu/client/CMakeFiles/kudu_client_test_util.dir/client-test-util.cc.o
15:16:19
/home/jenkins-slave/workspace/kudu-master/0/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:111:
error: cannot find symbol
15:16:19 .setIsAutoIncrementing(column.isAutoIncrementing())
15:16:19 ^
15:16:19 symbol: method setIsAutoIncrementing(boolean)
15:16:19 location: class Builder
15:16:19
/home/jenkins-slave/workspace/kudu-master/0/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:177:
error: cannot find symbol
15:16:19 .autoIncrementing(pb.getIsAutoIncrementing())
15:16:19 ^
15:16:19 symbol: method getIsAutoIncrementing()
15:16:19 location: variable pb of type ColumnSchemaPB
15:16:19
/home/jenkins-slave/workspace/kudu-master/0/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:209:
error: cannot find symbol
15:16:19 if (columnPb.getIsAutoIncrementing()) {
15:16:19 ^
15:16:19 symbol: method getIsAutoIncrementing()
15:16:19 location: variable columnPb of type ColumnSchemaPB
http://gerrit.cloudera.org:8080/#/c/19384/10/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/10/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@397
PS10, Line 397: /**
: * Sets if the column is auto-incrementing column. False by
default.
: * @param autoIncrementing a boolean that indicates if the
column is
: * an auto-incrementing column
: * @return this instance
: */
: public ColumnSchemaBuilder autoIncrementing(boolean
autoIncrementing) {
: this.autoIncrementing = autoIncrementing;
: return this;
: }
BTW, did you explore an alternative here: add a new helper class
AutoIncrementingColumnSchemaBuilder instead with only a subset of methods that
make sense for an auto-incrementing column and don't add this
ColumnSchemaBuilder.autoIncrementing() method?
The new helper would set the type and other related attributes for the column
automatically.
I guess going forward with that alternative would require corresponding change
in the C++ client as well, so it's not necessary to make the change in this
patch right away. Just curious whether it's a viable alternative from you
point of view.
Thanks!
http://gerrit.cloudera.org:8080/#/c/19384/10/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/10/java/kudu-client/src/main/java/org/apache/kudu/Schema.java@125
PS10, Line 125: findKey
nit: maybe, rename this into 'isKeyFound' for better readability?
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: // Make a copy of mutable list of column schema, then add
an auto-incrementing
: // column after the columns marked as key columns.
: List<ColumnSchema> colSchemas = new
ArrayList<>(schema.getColumns());
: Preconditions.checkNotNull(colSchemas);
> Yes, auto-incrementing column is added after all columns which are marked a
Thank you for the clarification!
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 isKeyUnique) {
> We need to mark all of key columns as non unique key columns, including the
Ah, I see -- it seems I missed the context of the call sites for this method
and was confused by the name of the new parameter.
Thanks for the update.
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@2631
PS6, Line 2631: columns.add(new ColumnSchema.ColumnSchemaBuilder("key",
Type.INT64)
> added new test cases
Thanks a lot!
--
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: 10
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: Wed, 04 Jan 2023 05:44:01 +0000
Gerrit-HasComments: Yes
