gaborkaszab commented on code in PR #7555:
URL: https://github.com/apache/iceberg/pull/7555#discussion_r1189841619
##########
api/src/main/java/org/apache/iceberg/ContentFile.java:
##########
@@ -113,6 +113,16 @@ default Integer sortOrderId() {
return null;
}
+ /** Returns the data sequence number of the snapshot in which the file
should be applied. */
+ default Long dataSequenceNumber() {
+ return null;
+ }
+
+ /** Returns the file sequence number. */
+ default Long fileSequenceNumber() {
+ return null;
Review Comment:
Thanks for pointing this out! I wasn't aware of these IndexedXXX classes. To
be consistent with the existing logic, I added the new functions too similarly
to specId.
##########
core/src/test/java/org/apache/iceberg/TableTestBase.java:
##########
@@ -410,11 +410,19 @@ void validateSnapshot(Snapshot old, Snapshot snap, Long
sequenceNumber, DataFile
if (sequenceNumber != null) {
V1Assert.assertEquals(
"Data sequence number should default to 0", 0,
entry.dataSequenceNumber().longValue());
+ V1Assert.assertEquals(
+ "Data sequence number should default to 0",
Review Comment:
I added a check for file sequence number.
However, I'm wondering if the checks for dataSequenceNumber are correct. If
I'm not mistaken if there was a rewrite then the dataSequenceNumber on the
ManifestEntry can differ from the one on the Snapshot. So these checks (even
the existing ones) are correct only if there were no rewrites on the table. Is
my understanding correct here?
##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -222,6 +227,24 @@ void setSpecId(int specId) {
this.partitionSpecId = specId;
}
+ @Override
+ public Long dataSequenceNumber() {
+ return this.dataSequenceNumber;
+ }
+
+ public void setDataSequenceNumber(Long dataSequenceNumber) {
+ this.dataSequenceNumber = dataSequenceNumber;
+ }
+
+ @Override
+ public Long fileSequenceNumber() {
+ return this.fileSequenceNumber;
Review Comment:
Done
##########
api/src/main/java/org/apache/iceberg/ContentFile.java:
##########
@@ -113,6 +113,16 @@ default Integer sortOrderId() {
return null;
}
+ /** Returns the data sequence number of the snapshot in which the file
should be applied. */
Review Comment:
Sorry, my bad. I sent some of my comments too early. I'm currently preparing
the patch before pushing it to review.
##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -222,6 +227,24 @@ void setSpecId(int specId) {
this.partitionSpecId = specId;
}
+ @Override
+ public Long dataSequenceNumber() {
+ return this.dataSequenceNumber;
Review Comment:
Indeed. Done
##########
core/src/test/java/org/apache/iceberg/TableTestBase.java:
##########
@@ -562,6 +544,44 @@ void validateDeleteManifest(
Assert.assertFalse("Should find all files in the manifest",
expectedFiles.hasNext());
}
+ private <T extends ContentFile<T>> void validateManifestSequenceNumbers(
Review Comment:
Good point. I added some additional tests on Snapshot and Scan levels.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
