Adar Dembo 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? http://gerrit.cloudera.org:8080/#/c/10574/1//COMMIT_MSG@11 PS1, Line 11: attribributes attributes 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 help here. 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, but I think it may be better to include it anyway, just so that someone reading this can understand that it's "dumb" (i.e. it just considers all fields in the class). 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? 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 order in equals()? I don't think it matters for correctness, but it makes it a lot easier to tell if a field is in one but not the other. 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 fields have to be compared/hashed too? 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? 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. 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(). 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, despite being equal as per equal(), they're not the same instance (if they were the same instance and equals() weren't overridden, assertNotSame would fail). -- 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: Kudu Jenkins Gerrit-Comment-Date: Fri, 01 Jun 2018 17:44:00 +0000 Gerrit-HasComments: Yes
