You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "ahmedabu98 (via GitHub)" <gi...@apache.org> on 2025/01/09 22:27:56 UTC
[PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
ahmedabu98 opened a new pull request, #33554:
URL: https://github.com/apache/beam/pull/33554
Adding a more accurate equals method for SerializedDataFile because the default equals method does not account for objects of type `Map<Integer, byte[]>`.
This helps clean up logs like these, which are actually extremely noisy:
`WARNING: Coder of type class org.apache.beam.sdk.schemas.SchemaCoder has a #structuralValue method which does not return true when the encoding of the elements is equal. Element FileWriteResult{tableIdentifierString={"namespace":["managed_iceberg_bqms_tests_no_delete"],"name":"testWriteToPartitionedTable_667244372263427"}, serializableDataFile=SerializableDataFile{path=gs://managed-iceberg-integration-tests/BigQueryMetastoreCatalogIT/408aefd5-2598-4fc3-b2e1-451500fff0c8/managed_iceberg_bqms_tests_no_delete.db/testWriteToPartitionedTable_667244372263427/data/bool=true/datetime_hour=1970-01-01-00/str_trunc=value_1/23583901-d389-4eca-a748-f7194ce06fbc_dae547f4-509e-4fd6-a805-b23cb94e2b55_1.parquet, fileFormat=PARQUET, recordCount=16, fileSizeInBytes=7052, partitionPath=bool=true/datetime_hour=0/str_trunc=value_1, partitionSpecId=0, keyMetadata=null, splitOffsets=[4], columnSizes={1=99, 2=106, 3=102, 4=45, 5=80, 9=91, 10=107, 11=83, 12=80, 13=83, 14=115, 16=80, 17=110, 18=121, 19=83,
20=151, 21=49, 23=49, 24=49, 25=49, 26=49}, valueCounts={1=16, 2=16, 3=16, 4=16, 5=16, 9=16, 10=16, 11=16, 12=16, 13=16, 14=16, 16=16, 17=16, 18=16, 19=16, 20=67, 21=16, 23=16, 24=16, 25=16, 26=16}, nullValueCounts={1=0, 2=0, 3=0, 4=0, 5=0, 9=0, 10=0, 11=0, 12=0, 13=0, 14=0, 16=0, 17=0, 18=0, 19=0, 20=3, 21=16, 23=16, 24=16, 25=16, 26=16}, nanValueCounts={17=0, 24=0}, lowerBounds={1=[B@1b0b6e85, 2=[B@76cb6b52, 3=[B@14752039, 4=[B@5b22e2ef, 5=[B@1b1dd048, 9=[B@36a1b982, 10=[B@be82387, 11=[B@47918d8f, 12=[B@32fb8aa5, 13=[B@9ad58d1, 14=[B@4313a678, 16=[B@825b660, 17=[B@66e5bb63, 18=[B@56d0cbe, 19=[B@5b135a0d}, upperBounds={1=[B@7ea061b9, 2=[B@35d5e3ad, 3=[B@58300155, 4=[B@19b2fb6c, 5=[B@64e2fc4a, 9=[B@7e30a3f0, 10=[B@26d22fca, 11=[B@2835d9e7, 12=[B@5bc66007, 13=[B@209267ac, 14=[B@1789b00b, 16=[B@1175b9fc, 17=[B@79db358d, 18=[B@4ac7821d, 19=[B@10146aa6}}}`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on PR #33554:
URL: https://github.com/apache/beam/pull/33554#issuecomment-2613067544
I'm OK to go ahead with this approach but it should be temporary. We shouldn't be implementing equals and hashcode manually for things like this. It is simply too error prone and is guaranteed to bite us.
Lets be sure this is done in a compatible way so we can migrate to types that "just work" with our various automations, and upgrade the automation to work with ByteBuffer. Basically Java arrays are to be avoided except in low-level cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33554:
URL: https://github.com/apache/beam/pull/33554#issuecomment-2581449837
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1941900281
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,105 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
+ && getFileFormat().equals(that.getFileFormat())
+ && getRecordCount() == that.getRecordCount()
+ && getFileSizeInBytes() == that.getFileSizeInBytes()
+ && getPartitionPath().equals(that.getPartitionPath())
+ && getPartitionSpecId() == that.getPartitionSpecId()
+ && Objects.equals(getKeyMetadata(), that.getKeyMetadata())
+ && Objects.equals(getSplitOffsets(), that.getSplitOffsets())
+ && Objects.equals(getColumnSizes(), that.getColumnSizes())
+ && Objects.equals(getValueCounts(), that.getValueCounts())
+ && Objects.equals(getNullValueCounts(), that.getNullValueCounts())
+ && Objects.equals(getNanValueCounts(), that.getNanValueCounts())
+ && mapEquals(getLowerBounds(), that.getLowerBounds())
+ && mapEquals(getUpperBounds(), that.getUpperBounds());
+ }
+
+ private static boolean mapEquals(
+ @Nullable Map<Integer, byte[]> map1, @Nullable Map<Integer, byte[]> map2) {
+ if (map1 == null && map2 == null) {
+ return true;
+ } else if (map1 == null || map2 == null) {
+ return false;
+ }
+ Equivalence<byte[]> byteArrayEquivalence =
+ new Equivalence<byte[]>() {
+ @Override
+ protected boolean doEquivalent(byte[] a, byte[] b) {
+ return Arrays.equals(a, b);
+ }
+
+ @Override
+ protected int doHash(byte[] bytes) {
+ return Arrays.hashCode(bytes);
+ }
+ };
+
+ return Maps.difference(map1, map2, byteArrayEquivalence).areEqual();
+ }
+
+ @Override
+ public final int hashCode() {
+ int hashCode = 1;
Review Comment:
Do you mean use `Objects.hashCode()` for each field?
FWIW I copied this over from the generated AutoValue hashcode logic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1943070952
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
Review Comment:
Hmmm maybe it was `Objects.hash`. Weird that Autovalue doesn't use it.
Basically, don't calculate a hash yourself but just say `Objects.hash(field1, field2, field3, ...)` to avoid coming up with your own, and making the logic very obvious.
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,105 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
+ && getFileFormat().equals(that.getFileFormat())
+ && getRecordCount() == that.getRecordCount()
+ && getFileSizeInBytes() == that.getFileSizeInBytes()
+ && getPartitionPath().equals(that.getPartitionPath())
+ && getPartitionSpecId() == that.getPartitionSpecId()
+ && Objects.equals(getKeyMetadata(), that.getKeyMetadata())
+ && Objects.equals(getSplitOffsets(), that.getSplitOffsets())
+ && Objects.equals(getColumnSizes(), that.getColumnSizes())
+ && Objects.equals(getValueCounts(), that.getValueCounts())
+ && Objects.equals(getNullValueCounts(), that.getNullValueCounts())
+ && Objects.equals(getNanValueCounts(), that.getNanValueCounts())
+ && mapEquals(getLowerBounds(), that.getLowerBounds())
+ && mapEquals(getUpperBounds(), that.getUpperBounds());
+ }
+
+ private static boolean mapEquals(
+ @Nullable Map<Integer, byte[]> map1, @Nullable Map<Integer, byte[]> map2) {
+ if (map1 == null && map2 == null) {
+ return true;
+ } else if (map1 == null || map2 == null) {
+ return false;
+ }
+ Equivalence<byte[]> byteArrayEquivalence =
+ new Equivalence<byte[]>() {
+ @Override
+ protected boolean doEquivalent(byte[] a, byte[] b) {
+ return Arrays.equals(a, b);
+ }
+
+ @Override
+ protected int doHash(byte[] bytes) {
+ return Arrays.hashCode(bytes);
+ }
+ };
+
+ return Maps.difference(map1, map2, byteArrayEquivalence).areEqual();
+ }
+
+ @Override
+ public final int hashCode() {
+ int hashCode = 1;
Review Comment:
Hmmm maybe it was `Objects.hash`. Weird that Autovalue doesn't use it.
Basically, don't calculate a hash yourself but just say `Objects.hash(field1, field2, field3, ...)` to avoid coming up with your own, and making the logic very obvious.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33554:
URL: https://github.com/apache/beam/pull/33554#issuecomment-2604563724
Reminder, please take a look at this pr: @kennknowles @damccorm @Abacn
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1951111314
##########
sdks/java/io/iceberg/src/test/java/org/apache/beam/sdk/io/iceberg/SerializableDataFileTest.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.iceberg;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+/**
+ * Test for {@link SerializableDataFile}. More tests can be found in {@link
+ * org.apache.beam.sdk.io.iceberg.RecordWriterManagerTest}.
+ */
+public class SerializableDataFileTest {
+ static final Set<String> FIELDS_SET =
+ ImmutableSet.<String>builder()
+ .add("path")
+ .add("fileFormat")
+ .add("recordCount")
+ .add("fileSizeInBytes")
+ .add("partitionPath")
+ .add("partitionSpecId")
+ .add("keyMetadata")
+ .add("splitOffsets")
+ .add("columnSizes")
+ .add("valueCounts")
+ .add("nullValueCounts")
+ .add("nanValueCounts")
+ .add("lowerBounds")
+ .add("upperBounds")
+ .build();
+
+ @Test
+ public void testFieldsInEqualsMethodInSyncWithGetterFields() {
Review Comment:
We talked offline, and I think using `EqualityTester` could also be a good way to do this. Though if all the examples we pass to it have the default value for new fields, it will still miss errors...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33554:
URL: https://github.com/apache/beam/pull/33554#issuecomment-2581561941
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @kennknowles for label java.
R: @damccorm for label build.
R: @Abacn for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1910661020
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
Review Comment:
Don't suppress this. You really need to address it.
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
+ && getFileFormat().equals(that.getFileFormat())
+ && getRecordCount() == that.getRecordCount()
+ && getFileSizeInBytes() == that.getFileSizeInBytes()
+ && getPartitionPath().equals(that.getPartitionPath())
+ && getPartitionSpecId() == that.getPartitionSpecId()
+ && Objects.equals(getKeyMetadata(), that.getKeyMetadata())
+ && Objects.equals(getSplitOffsets(), that.getSplitOffsets())
+ && Objects.equals(getColumnSizes(), that.getColumnSizes())
+ && Objects.equals(getValueCounts(), that.getValueCounts())
+ && Objects.equals(getNullValueCounts(), that.getNullValueCounts())
+ && Objects.equals(getNanValueCounts(), that.getNanValueCounts())
+ && mapEquals(getLowerBounds(), that.getLowerBounds())
+ && mapEquals(getUpperBounds(), that.getUpperBounds());
+ }
+
+ private static boolean mapEquals(
+ @Nullable Map<Integer, byte[]> map1, @Nullable Map<Integer, byte[]> map2) {
Review Comment:
The map isn't the issue, it is the byte[]. There may be an alternative byte buffer data structure that would have a structural equals.
Otherwise, what you need to do for `structuralValue` is wrap any mutable array. It is quite a pain.
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
Review Comment:
You are sure that autovalue `equals` doesn't handle this?
I am concerned that it is a lot of toil and error-prone to manage this list of calls. It really should be automated by autovalue.
Is there a way to combine any auto-generated `equals` with just the slight changes you intend?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1913413017
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
Review Comment:
The generated AutoValue equals method does a naive Map::equals operation, so it just checks reference equality for the byte arrays instead of the contents.
> Is there a way to combine any auto-generated equals with just the slight changes you intend?
I'm not sure, is there a way to do this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1913412489
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
Review Comment:
Thanks, I checked and saw that indeed the AutoValue hashcode is also implemented naively.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on PR #33554:
URL: https://github.com/apache/beam/pull/33554#issuecomment-2612917113
@kennknowles could you take another look at this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33554:
URL: https://github.com/apache/beam/pull/33554#issuecomment-2609658580
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @robertwb for label java.
R: @damccorm for label build.
R: @chamikaramj for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1943070952
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
Review Comment:
Hmmm maybe it was `Objects.hash`. Check the generated autovalue.
Basically, don't calculate a hash yourself but just say `Objects.hash(field1, field2, field3, ...)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 merged PR #33554:
URL: https://github.com/apache/beam/pull/33554
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1949943171
##########
sdks/java/io/iceberg/src/test/java/org/apache/beam/sdk/io/iceberg/SerializableDataFileTest.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.iceberg;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+/**
+ * Test for {@link SerializableDataFile}. More tests can be found in {@link
+ * org.apache.beam.sdk.io.iceberg.RecordWriterManagerTest}.
+ */
+public class SerializableDataFileTest {
+ static final Set<String> FIELDS_SET =
+ ImmutableSet.<String>builder()
+ .add("path")
+ .add("fileFormat")
+ .add("recordCount")
+ .add("fileSizeInBytes")
+ .add("partitionPath")
+ .add("partitionSpecId")
+ .add("keyMetadata")
+ .add("splitOffsets")
+ .add("columnSizes")
+ .add("valueCounts")
+ .add("nullValueCounts")
+ .add("nanValueCounts")
+ .add("lowerBounds")
+ .add("upperBounds")
+ .build();
+
+ @Test
+ public void testFieldsInEqualsMethodInSyncWithGetterFields() {
Review Comment:
@kennknowles added a check here with a helpful error message
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1949925850
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,105 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
+ && getFileFormat().equals(that.getFileFormat())
+ && getRecordCount() == that.getRecordCount()
+ && getFileSizeInBytes() == that.getFileSizeInBytes()
+ && getPartitionPath().equals(that.getPartitionPath())
+ && getPartitionSpecId() == that.getPartitionSpecId()
+ && Objects.equals(getKeyMetadata(), that.getKeyMetadata())
+ && Objects.equals(getSplitOffsets(), that.getSplitOffsets())
+ && Objects.equals(getColumnSizes(), that.getColumnSizes())
+ && Objects.equals(getValueCounts(), that.getValueCounts())
+ && Objects.equals(getNullValueCounts(), that.getNullValueCounts())
+ && Objects.equals(getNanValueCounts(), that.getNanValueCounts())
+ && mapEquals(getLowerBounds(), that.getLowerBounds())
+ && mapEquals(getUpperBounds(), that.getUpperBounds());
+ }
+
+ private static boolean mapEquals(
+ @Nullable Map<Integer, byte[]> map1, @Nullable Map<Integer, byte[]> map2) {
+ if (map1 == null && map2 == null) {
+ return true;
+ } else if (map1 == null || map2 == null) {
+ return false;
+ }
+ Equivalence<byte[]> byteArrayEquivalence =
+ new Equivalence<byte[]>() {
+ @Override
+ protected boolean doEquivalent(byte[] a, byte[] b) {
+ return Arrays.equals(a, b);
+ }
+
+ @Override
+ protected int doHash(byte[] bytes) {
+ return Arrays.hashCode(bytes);
+ }
+ };
+
+ return Maps.difference(map1, map2, byteArrayEquivalence).areEqual();
+ }
+
+ @Override
+ public final int hashCode() {
+ int hashCode = 1;
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1929024339
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,105 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
+ && getFileFormat().equals(that.getFileFormat())
+ && getRecordCount() == that.getRecordCount()
+ && getFileSizeInBytes() == that.getFileSizeInBytes()
+ && getPartitionPath().equals(that.getPartitionPath())
+ && getPartitionSpecId() == that.getPartitionSpecId()
+ && Objects.equals(getKeyMetadata(), that.getKeyMetadata())
+ && Objects.equals(getSplitOffsets(), that.getSplitOffsets())
+ && Objects.equals(getColumnSizes(), that.getColumnSizes())
+ && Objects.equals(getValueCounts(), that.getValueCounts())
+ && Objects.equals(getNullValueCounts(), that.getNullValueCounts())
+ && Objects.equals(getNanValueCounts(), that.getNanValueCounts())
+ && mapEquals(getLowerBounds(), that.getLowerBounds())
+ && mapEquals(getUpperBounds(), that.getUpperBounds());
+ }
+
+ private static boolean mapEquals(
+ @Nullable Map<Integer, byte[]> map1, @Nullable Map<Integer, byte[]> map2) {
+ if (map1 == null && map2 == null) {
+ return true;
+ } else if (map1 == null || map2 == null) {
+ return false;
+ }
+ Equivalence<byte[]> byteArrayEquivalence =
+ new Equivalence<byte[]>() {
+ @Override
+ protected boolean doEquivalent(byte[] a, byte[] b) {
+ return Arrays.equals(a, b);
+ }
+
+ @Override
+ protected int doHash(byte[] bytes) {
+ return Arrays.hashCode(bytes);
+ }
+ };
+
+ return Maps.difference(map1, map2, byteArrayEquivalence).areEqual();
+ }
+
+ @Override
+ public final int hashCode() {
+ int hashCode = 1;
Review Comment:
use `Objects.hashCode` I'd say
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1943070952
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
Review Comment:
Hmmm maybe it was `Objects.hash`. Weird that Autovalue doesn't use it.
Basically, don't calculate a hash yourself but just say `Objects.hash(field1, field2, field3, ...)` to avoid coming up with your own, and making the logic very obvious.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33554:
URL: https://github.com/apache/beam/pull/33554#issuecomment-2647813673
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @Abacn for label java.
R: @Abacn for label build.
R: @Abacn for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1951114688
##########
sdks/java/io/iceberg/src/test/java/org/apache/beam/sdk/io/iceberg/SerializableDataFileTest.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.iceberg;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+/**
+ * Test for {@link SerializableDataFile}. More tests can be found in {@link
+ * org.apache.beam.sdk.io.iceberg.RecordWriterManagerTest}.
+ */
+public class SerializableDataFileTest {
+ static final Set<String> FIELDS_SET =
+ ImmutableSet.<String>builder()
+ .add("path")
+ .add("fileFormat")
+ .add("recordCount")
+ .add("fileSizeInBytes")
+ .add("partitionPath")
+ .add("partitionSpecId")
+ .add("keyMetadata")
+ .add("splitOffsets")
+ .add("columnSizes")
+ .add("valueCounts")
+ .add("nullValueCounts")
+ .add("nanValueCounts")
+ .add("lowerBounds")
+ .add("upperBounds")
+ .build();
+
+ @Test
+ public void testFieldsInEqualsMethodInSyncWithGetterFields() {
Review Comment:
Right, `EqualsTester` just simplifies testing the equals() method for different objects, but it does not help us determine if all fields are accounted for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #33554:
URL: https://github.com/apache/beam/pull/33554#discussion_r1913413723
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/SerializableDataFile.java:
##########
@@ -199,4 +203,53 @@ DataFile createDataFile(Map<Integer, PartitionSpec> partitionSpecs) {
}
return output;
}
+
+ @Override
+ @SuppressWarnings("EqualsHashCode")
+ public final boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SerializableDataFile that = (SerializableDataFile) o;
+ return getPath().equals(that.getPath())
+ && getFileFormat().equals(that.getFileFormat())
+ && getRecordCount() == that.getRecordCount()
+ && getFileSizeInBytes() == that.getFileSizeInBytes()
+ && getPartitionPath().equals(that.getPartitionPath())
+ && getPartitionSpecId() == that.getPartitionSpecId()
+ && Objects.equals(getKeyMetadata(), that.getKeyMetadata())
+ && Objects.equals(getSplitOffsets(), that.getSplitOffsets())
+ && Objects.equals(getColumnSizes(), that.getColumnSizes())
+ && Objects.equals(getValueCounts(), that.getValueCounts())
+ && Objects.equals(getNullValueCounts(), that.getNullValueCounts())
+ && Objects.equals(getNanValueCounts(), that.getNanValueCounts())
+ && mapEquals(getLowerBounds(), that.getLowerBounds())
+ && mapEquals(getUpperBounds(), that.getUpperBounds());
+ }
+
+ private static boolean mapEquals(
+ @Nullable Map<Integer, byte[]> map1, @Nullable Map<Integer, byte[]> map2) {
Review Comment:
Right, ideally we'd be using ByteBuffer, but that currentl raises other issues. I saw your comment on the other PR and replied [here](https://github.com/apache/beam/pull/32821#issuecomment-2587384084).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Managed Iceberg] custom equals method for SerializedDataFile [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33554:
URL: https://github.com/apache/beam/pull/33554#issuecomment-2628929143
Reminder, please take a look at this pr: @robertwb @damccorm @chamikaramj
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org