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 18: (16 comments) http://gerrit.cloudera.org:8080/#/c/19384/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19384/18//COMMIT_MSG@11 PS18, Line 11: auto_increment_id auto_incrementing_id http://gerrit.cloudera.org:8080/#/c/19384/18//COMMIT_MSG@27 PS18, Line 27: end-end nit: end-to-end ? http://gerrit.cloudera.org:8080/#/c/19384/18/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/18/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@159 PS18, Line 159: Type type = Type.getTypeForDataType(pb.getType()); : ColumnTypeAttributes typeAttributes = pb.hasTypeAttributes() ? : pbToColumnTypeAttributes(pb.getTypeAttributes()) : null; : Object defaultValue = pb.hasWriteDefaultValue() ? : byteStringToObject(type, typeAttributes, pb.getWriteDefaultValue()) : null; nit: could move these lines down to be after line 175? http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@167 PS18, Line 167: int desiredBlockSize = pb.getCfileBlockSize(); nit: this could be moved down to be after line 175 as well? http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@170 PS18, Line 170: default encoding Why default? It seems they are coming from the 'pb' parameter. http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@171 PS18, Line 171: return new ColumnSchema.AutoIncrementingColumnSchemaBuilder() : .encoding(encoding) : .compressionAlgorithm(compressionAlgorithm) : .build(); After looking at this one more time I realized I'm confused :) It seems in this case we are discarding whatever is set in the other fields of the 'pb' parameter. Why is so? I'd think this helper method takes all the fields in the 'pb' as the input, treating those as the source of truth, but only one extra artificial field is set: nonUniqueKey(). It seems now the semantics of this helper method has changed. It would be great to clarify why we need to change it up to such extent. Thanks! http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@179 PS18, Line 179: !isKeyUnique nit: remove the negation and swap lines 180 & 182? http://gerrit.cloudera.org:8080/#/c/19384/18/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/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@792 PS18, Line 792: schema nit: a schema http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@798 PS18, Line 798: assertEquals(1, schema.getPrimaryKeyColumnCount()); This now looks a bit different from we have in the C++ client: https://gerrit.cloudera.org/#/c/19272/19/src/kudu/client/client-unittest.cc@233 Should we reconcile? http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@799 PS18, Line 799: table nit: a table http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@810 PS18, Line 810: assertEquals(3, schema.getColumnCount()); : assertEquals(2, schema.getPrimaryKeyColumnCount()); Isn't this some sort of a contradiction to the POLA principle (https://en.wikipedia.org/wiki/Principle_of_least_astonishment)? At lines 797 & 798 the schema shows different numbers. I'd expect those to be the same. http://gerrit.cloudera.org:8080/#/c/19384/18/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/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2570 PS18, Line 2570: schema nit for here and elsewhere in the code below: a schema http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2579 PS18, Line 2579: assertFalse(schema.isPrimaryKeyUnique()); Could you also add an assert to verify the number of columns in the resulting table's schema? http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2580 PS18, Line 2580: 2, schema.getPrimaryKeyColumnCount() Now this behavior looks a bit different from what we have in the C++ client: https://gerrit.cloudera.org/#/c/19272/19/src/kudu/client/client-unittest.cc@233 Should we reconcile? http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2610 PS18, Line 2610: assertTrue(schema.hasAutoIncrementingColumn()); Could you also add an assertion on the number of columns in the resulting table's schema? http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2630 PS18, Line 2630: } Could you also add a test where a new table with a single column is created, but the only column is the auto-incremented column (i.e. produced with ColumnSchema.AutoIncrementingColumnSchemaBuilder())? A couple of sub-scenarios would be: * the only column is set to be the unique primary key * the only column is set to be the non-unique primary key -- 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: 18 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Wed, 18 Jan 2023 04:39:31 +0000 Gerrit-HasComments: Yes
