Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10574 )
Change subject: [java] Add equals and hashcode to Schema ...................................................................... Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/10574/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10574/1//COMMIT_MSG@7 PS1, Line 7: [java] Add equals and hashcode to Schema > Why do you need these new methods? This simplifies testing backup and restore. I can easily validate that the round trip creates the same schema. It should also help detect if any new field/feature is missing in the backup restore tests. http://gerrit.cloudera.org:8080/#/c/10574/1//COMMIT_MSG@11 PS1, Line 11: attribributes > attributes Done http://gerrit.cloudera.org:8080/#/c/10574/1//COMMIT_MSG@13 PS1, Line 13: A few light tests were added to prove equality works. > Could you add a few tests for hashCode()? Hash-based collections ought to h Given how trivial the implementation is, it seamed like it would basically be testing Guava and not add much value. http://gerrit.cloudera.org:8080/#/c/10574/1/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/10574/1/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@195 PS1, Line 195: @Override > You don't need to consider typeSize since it's derived from typeAttributes, Done http://gerrit.cloudera.org:8080/#/c/10574/1/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@200 PS1, Line 200: return key == that.key && > Nit: for consistency, can you use Objects.equal() for every comparison? Done http://gerrit.cloudera.org:8080/#/c/10574/1/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@213 PS1, Line 213: return Objects.hashCode(name, type, key, nullable, defaultValue, desiredBlockSize, > Nit: can you change the hashing order to be the same as the comparison orde Done http://gerrit.cloudera.org:8080/#/c/10574/1/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/10574/1/java/kudu-client/src/main/java/org/apache/kudu/Schema.java@297 PS1, Line 297: > Why is it sufficient to consider columnsByIndex? Why don't the other class All the other fields are derived from the list of columns passed to the constructor which is effectively columnsByIndex. http://gerrit.cloudera.org:8080/#/c/10574/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionSchema.java File java/kudu-client/src/main/java/org/apache/kudu/client/PartitionSchema.java: http://gerrit.cloudera.org:8080/#/c/10574/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionSchema.java@106 PS1, Line 106: @Override > Why don't equals()/hashCode() need to consider isSimple? I thought it was derived from the other fields, but technically the schema is needed to derive it. So I will add it. In the future I think isSimple is likely better treated as a method than a field given it depends on the Schema. http://gerrit.cloudera.org:8080/#/c/10574/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionSchema.java@191 PS1, Line 191: return numBuckets == that.numBuckets && > Nit: use Objects.equal() throughout for consistency. Done http://gerrit.cloudera.org:8080/#/c/10574/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionSchema.java@198 PS1, Line 198: return Objects.hashCode(columnIds, numBuckets, seed); > Nit: use the same order for both equals() and hashCode(). Done http://gerrit.cloudera.org:8080/#/c/10574/1/java/kudu-client/src/test/java/org/apache/kudu/TestSchema.java File java/kudu-client/src/test/java/org/apache/kudu/TestSchema.java: http://gerrit.cloudera.org:8080/#/c/10574/1/java/kudu-client/src/test/java/org/apache/kudu/TestSchema.java@32 PS1, Line 32: assertEquals(BaseKuduTest.getBasicSchema(), BaseKuduTest.getBasicSchema()); : assertEquals(BaseKuduTest.getSchemaWithAllTypes(), BaseKuduTest.getSchemaWithAllTypes()); > For these two, you may want to throw in an assertNotSame to show that, desp Done -- To view, visit http://gerrit.cloudera.org:8080/10574 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0a6914b9b25df9e327ed6ae6bd61eac29c9c524c Gerrit-Change-Number: 10574 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Fri, 01 Jun 2018 18:07:49 +0000 Gerrit-HasComments: Yes
