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 6: (20 comments) 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 naming of the column in the comments? I saw at least three variants in this patch: * auto incrementing * auto_incrementing * auto-incrementing Probably, we should use 'auto-incrementing' since that's what is used in https://gerrit.cloudera.org/#/c/19097 ? 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 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 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 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 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 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 columns, not all the columns, no? 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 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.ColumnSchemaPB has the 'is_auto_incrementing' field? https://gerrit.cloudera.org/#/c/19097/12/src/kudu/common/common.proto@142 If this parameter is taken into the consideration when translating Common.ColumnSchemaPB into run-time structures, what's the semantics of the Common.ColumnSchemaPB.is_auto_incrementing field in 'pb' passed as the first parameter then? I'm confused. 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 to verify difference for columns which are different only in non-unique-key property, not mixing in anything else? 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 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-incrementing and non-unique-key properties on columns? 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: auto_incrementing nit: auto-incrementing 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 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 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 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 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. 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. 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: * schema with auto-incrementing column which isn't marked with nonUniqueKey(true) * schema with auto-incrementing column which is marked with with key(true) * schema with auto-incrementing column which is marked as nullable * schema with auto-incrementing column which is marked as immutable * schema with auto-incrementing column with type String or INT32 (instead of INT64) -- 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: Fri, 23 Dec 2022 01:54:34 +0000 Gerrit-HasComments: Yes
