Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10574 )
Change subject: [java] Add equals and hashcode to Schema ...................................................................... Patch Set 4: (5 comments) > Patch Set 4: > > > it may be useful to be able to create a schema with column names and types, > > and compare that to the schema of an opened table to determine if rows can > > be written to it. > > I view this as compatibility not equality. One could argue that the definition of equality in this patch is really just compatibility for the backup feature/API. The meaning of equality is context dependent, and there are a lot of different contexts that the Schema class is used in. > > Or a different situation is the backup tool where exact equality is > > required including column IDs and column attributes. > > Why are columnIDs important in a backup tool? I think I need to look into > these ids more. I mistakenly assumed they were important, but you would know better than I. > > This also neatly sidesteps having to provide a .hashCode method, which I > > think should be avoided. > > Why should it be avoided? I've never seen a legitimate use for putting a Schema into a map or set container, and certainly never based on anything but reference equality. http://gerrit.cloudera.org:8080/#/c/10574/4/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/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@20 PS4, Line 20: import com.google.common.base.Objects; java.util.Objects is preferred. http://gerrit.cloudera.org:8080/#/c/10574/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@211 PS4, Line 211: // typeSize isn't required but included for simplicity I'd omit it, since other derived fields are omitted elsewhere such as in Schema. http://gerrit.cloudera.org:8080/#/c/10574/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@218 PS4, Line 218: private boolean defaultValueEquals(Object thisDefaultValue, Object thatDefaultValue) { Pretty sure this is equivalent to Objects.deepEquals http://gerrit.cloudera.org:8080/#/c/10574/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@231 PS4, Line 231: defaultValue This isn't handling byte[] instances properly. http://gerrit.cloudera.org:8080/#/c/10574/4/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/4/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionSchema.java@113 PS4, Line 113: Objects.equal(isSimple, schema.isSimple); isSimple can be omitted, since it's derived from the other two fields. -- 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: 4 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 05 Jun 2018 23:09:03 +0000 Gerrit-HasComments: Yes
