[GitHub] incubator-hawq pull request #1365: Adding equals() to Fragment, to facilitat...
Github user mgoddard-pivotal closed the pull request at: https://github.com/apache/incubator-hawq/pull/1365 ---
[GitHub] incubator-hawq pull request #1365: Adding equals() to Fragment, to facilitat...
Github user shivzone commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1365#discussion_r188471507
--- Diff: pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/Fragment.java
---
@@ -140,4 +142,48 @@ public String getProfile() {
public void setProfile(String profile) {
this.profile = profile;
}
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + index;
+ result = prime * result + Arrays.hashCode(metadata);
+ result = prime * result + ((profile == null) ? 0 :
profile.hashCode());
+ result = prime * result + Arrays.hashCode(replicas);
+ result = prime * result + ((sourceName == null) ? 0 :
sourceName.hashCode());
+ result = prime * result + Arrays.hashCode(userData);
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
--- End diff --
Not about correctness ... more about general java coding style. Not ideal
to have too many condition checks with return statements
---
[GitHub] incubator-hawq pull request #1365: Adding equals() to Fragment, to facilitat...
Github user mgoddard-pivotal commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1365#discussion_r188469835
--- Diff: pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/Fragment.java
---
@@ -140,4 +142,48 @@ public String getProfile() {
public void setProfile(String profile) {
this.profile = profile;
}
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + index;
+ result = prime * result + Arrays.hashCode(metadata);
+ result = prime * result + ((profile == null) ? 0 :
profile.hashCode());
+ result = prime * result + Arrays.hashCode(replicas);
+ result = prime * result + ((sourceName == null) ? 0 :
sourceName.hashCode());
+ result = prime * result + Arrays.hashCode(userData);
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
--- End diff --
If it's the former, I should probably create a unit test to verify the
behavior. If it's the latter, could you point me to some precedents within
this codebase?
---
[GitHub] incubator-hawq pull request #1365: Adding equals() to Fragment, to facilitat...
Github user mgoddard-pivotal commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1365#discussion_r188469685
--- Diff: pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/Fragment.java
---
@@ -140,4 +142,48 @@ public String getProfile() {
public void setProfile(String profile) {
this.profile = profile;
}
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + index;
+ result = prime * result + Arrays.hashCode(metadata);
+ result = prime * result + ((profile == null) ? 0 :
profile.hashCode());
+ result = prime * result + Arrays.hashCode(replicas);
+ result = prime * result + ((sourceName == null) ? 0 :
sourceName.hashCode());
+ result = prime * result + Arrays.hashCode(userData);
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
--- End diff --
Well, I just had my IDE, Eclipse, generate these two methods for me. Line
161 does what you have listed there, your top line. Line 165 performs the same
check you have there as `if (o instancef Point) {`, then `obj` is cast as
Fragment on line 167. The remainder of the logic you have within the complex
`... && ...` statement is equivalently expressed in the remaining checks, but
within the `if ... else if ... else` clauses. Is your comment meant to point
out an issue with correctness, or more of an aesthetic issue?
---
[GitHub] incubator-hawq pull request #1365: Adding equals() to Fragment, to facilitat...
Github user shivzone commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1365#discussion_r188406131
--- Diff: pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/Fragment.java
---
@@ -140,4 +142,48 @@ public String getProfile() {
public void setProfile(String profile) {
this.profile = profile;
}
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + index;
+ result = prime * result + Arrays.hashCode(metadata);
+ result = prime * result + ((profile == null) ? 0 :
profile.hashCode());
+ result = prime * result + Arrays.hashCode(replicas);
+ result = prime * result + ((sourceName == null) ? 0 :
sourceName.hashCode());
+ result = prime * result + Arrays.hashCode(userData);
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
--- End diff --
Few comments about how the equals method has been implemented.
Use obj instanceOf Fragment. Also you can simplify this by simply using a
single statement with && using Objects.equals() for Strings and either
Arrays.equals or Objects.deepEquals for arrays
if (this == obj) return true;
if (o instanceof Point) {
Fragment other = (Fragment)other;
return (index == other.x) && Arrays.equals(metadata,other.metadata) &&
Objects.equals(profile,other.profile) && .;
}
return false;
---
[GitHub] incubator-hawq pull request #1365: Adding equals() to Fragment, to facilitat...
GitHub user mgoddard-pivotal opened a pull request: https://github.com/apache/incubator-hawq/pull/1365 Adding equals() to Fragment, to facilitate comparisons in tests Writing some unit tests for the PXF + S3 + Parquet code, and the equals() was necessary. Also, added hashCode() to this class. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mgoddard-pivotal/incubator-hawq add-fragment-equals Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-hawq/pull/1365.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1365 commit 5a431fa477e9633a706b700369e771058b3c59bd Author: Michael Goddard Date: 2018-05-14T21:24:55Z Adding an equals() method to Fragment to facilitate tests involving methods returning Fragment commit 12682835dbc8eb779812cbb8b1cf36f15ed974da Author: Michael Goddard Date: 2018-05-15T00:03:08Z Revert "Adding an equals() method to Fragment to facilitate tests involving methods returning Fragment" This reverts commit 5a431fa477e9633a706b700369e771058b3c59bd. commit 00ae9ba688708cb8cbb0b2e9f43ddcf8f88182e7 Author: Michael Goddard Date: 2018-05-15T15:34:27Z Adding equals() and hashCode() to Fragment, to facilitate comparisons in tests ---
