You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2026/05/09 16:46:02 UTC
[PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
amogh-jahagirdar opened a new pull request, #16263:
URL: https://github.com/apache/iceberg/pull/16263
We observed some cases where when EXISTING entries are carried over in an operation as part of a manifest merge, the first row ID assignment is not preserved which is unexpected.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221851327
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
+ table.updateProperties().set(TableProperties.MANIFEST_MIN_MERGE_COUNT, "1").commit();
+ table.newRewrite().deleteFile(FILE_A).addFile(FILE_C).commit();
Review Comment:
I find it a little misleading that `FILE_A` is removed in order to change the status of `FILE_B` to `EXISTING` in the same commit that adds `FILE_C`. I think it would be better to separate those two operations so that we can isolate and test the result of changing `FILE_B`. We can show that it is assigned a specific `first_row_id` from the previous read. Then after adding `FILE_C` and compacting the metadata we can assert that the same ID is still present.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222295255
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -416,13 +429,17 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
return entry;
}
};
+ } else if (!committed) {
+ // Preserve the entry with its firstRowId
+ // as this is an uncommitted manifest which may have already assigned row IDs (e.g. for
+ // EXISTING entries)
+ return Function.identity();
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
+ // committed pre-v3 manifest with a null manifest-level firstRowId
Review Comment:
for pre-v3 manifest files, shouldn't data file's first_row_id be null anyway? that means `identity` also works.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221248944
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
curious, maybe we can also add a direct UT in TestManifestReader directly to ensure coverage of idAssigner on both if/else branch?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3220256497
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,8 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
Review Comment:
Yeah @stevenzwu that's my analysis of all the call points as well. Fundamentally, I would expect the idTransformer that's set by default on the ManifestReader should do *no* transformation of the first row ID on the entry. That implicitly covers the 3 cases you mentioned.
And for the coppyAppendManifest case, if we really want to be defensive, I think the right solution is that the public writer APIs Iceberg exposes should suppress the firstRowId assignment on the ADDED entry. The read side should just consume what's there in my opinion.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#issuecomment-4422876368
Cheng did a lot of debugging on this issue which surfaced this problem, marking him as co-author. Thank you @ChengJiX!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221825939
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
Review Comment:
It would be helpful to have a comment in this test case that explains what is being triggered by setting the min merge count.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3213456577
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,8 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
Review Comment:
I'm still checking if changing idAssigner like this for all cases is really the right way to fix this or if during manifest merging we should actually pass in our own id assignment transformer that preserves the existing case.
Here we're changing the manifest reader because
```
// data file's first_row_id is null when the manifest's first_row_id is null
```
isn't true in the context of reading filtered manifests that have EXISTING entries which need to have first row IDs preserved but the filtered manifest does not have a first row ID.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3228815902
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
> I do, but I think I'm currently overruled by the majority. I don't like it when we change objects to "fix" them
I think this is reasonable. I'm okay with a precondition if that's what others think we should do. My argument for the opposite is that usually when we _can_ read correctly, we should. We know that in the case where there is no manifest `first_row_id` that this should be null. Is it worth failing a read?
It might be. Russell is correct that it signals a problem somewhere. It probably doesn't matter much either way, since this should be rare.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221248944
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
curious, maybe we can also add a direct UT in TestManifestReader to cover idAssigner on both if/else branch?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221881982
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
Okay, from going through the repro test, I think this is a legitimate case because manifest compaction is using a `ManifestFile` to read that doesn't have an assigned `first_row_id`. That means the assumption here is violated because we can have manifests that don't have an assigned `first_row_id` and we should still read and pass through the `first_row_id` from files.
I think the fix needs to distinguish between these cases. We need to have an `idAssigner` for committed manifests (this one) and an `idAssigner` used for uncommitted manifests that preserves the `first_row_id` like this does. We can't commit this fix because it breaks the defensive assignment we added for the v2/v1 snapshot case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222426321
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -192,7 +196,8 @@ private ManifestFile createManifest(int specId, List<ManifestFile> bin) {
boolean threw = true;
try {
for (ManifestFile manifest : bin) {
- try (ManifestReader<F> reader = newManifestReader(manifest)) {
+ boolean committed = snapshotId() != manifest.snapshotId();
Review Comment:
I think ` manifest.snapshotId() != null` should be a part of this (i.e. appendManifest) may add a manifest without a snapshot ID but that's uncommitted.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3228824164
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -1250,7 +1250,13 @@ protected ManifestWriter<DataFile> newManifestWriter(PartitionSpec manifestSpec)
@Override
protected ManifestReader<DataFile> newManifestReader(ManifestFile manifest) {
- return MergingSnapshotProducer.this.newManifestReader(manifest);
+ return newManifestReader(manifest, true);
Review Comment:
I'm fine with this default, but we should probably start requiring `isCommitted` to be passed. Can we deprecate the old one and start using this exclusively?
I'm fine with this as a follow-up since we want to unblock the release.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3229181311
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,33 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
Review Comment:
Ok so I've added 2 tests to ManifestReader; 1 for testing the uncommitted read path which preserves firstRowId (same path already being tested in the TestRowLinegeAssignment) case, and 1 for testing the committed manifest read path where we expect to null out the entry's first row ID. Since we're now distinguishing between uncommitted and committed, we have a good abstraction boundary to test.
The committed test is something that would detect if the behavior to defensively null out entries changed (verified by commenting out the most recent changes we made, and going back to what we had before).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3215150246
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +782,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
Review Comment:
Understand the logic a little bit: seems during merge or filter manifest, we may have null first_row_id at manifest while the existing file already has row_id which should be kept.
Tried for filtering case and the fix works. Maybe we can add the test coverage for filtering as well.
// apply a row filter so the read happens through the "filtered manifest" path
reader.filterRows(Expressions.greaterThan("id", 0));
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222632101
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -416,13 +429,17 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
return entry;
}
};
+ } else if (!committed) {
+ // Preserve the entry with its firstRowId
+ // as this is an uncommitted manifest which may have already assigned row IDs (e.g. for
+ // EXISTING entries)
+ return Function.identity();
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
+ // committed pre-v3 manifest with a null manifest-level firstRowId
Review Comment:
So `identity` logically works because we expect the entry will be null, but my understanding from @rdblue @RussellSpitzer is that the intent of the previous logic is to defensively assign the values regardless for the older manifests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222619132
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -1250,7 +1250,13 @@ protected ManifestWriter<DataFile> newManifestWriter(PartitionSpec manifestSpec)
@Override
protected ManifestReader<DataFile> newManifestReader(ManifestFile manifest) {
- return MergingSnapshotProducer.this.newManifestReader(manifest);
+ return newManifestReader(manifest, true);
Review Comment:
Preserves the existing behavior for newManifestReader. While the "isCommitted" is only passed through for the Data file manifest merging case, it's a bit more involved to just put it on that implementation rather than the base class. I think this is the cleanest way (and besides this is all private/package private classes).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221746387
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
I'm not sure about this change.
It looks like this case covers when the manifest's `first_row_id` is null. That should only happen when reading a snapshot from an older version without row IDs. In that case, the snapshot's `first-row-id` is null and we don't assign `first_row_id` to manifests or to data files. This is covered by [Row Lineage for Upgraded Tables](https://iceberg.apache.org/spec/#row-lineage-for-upgraded-tables) in the spec.
If `first-row-id` is assigned, then every manifest should have a `first_row_id` assigned. Then this branch shouldn't be triggering.
Is it possible that the problem is further up and a manifest is somehow missing a `first_row_id`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3227948840
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,33 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
Review Comment:
So there's a nuance to "change the behavior" which is why all the tests in the previous commit passed. Even with the previous commit, we ended up correctly setting row ID to null because the entries in older manifest wouldn't even have a first row ID to begin with, so the behavior was preserved for the main case we care about for older manifests, and there's quite a few tests there for the upgrade path.
The case we're talking about though is a case where we (somehow?) produce a pre-v3 manifest with first row IDs and we should ignore those by defensively setting those to null; in that specific case, there was a behavior change because we'd just keep those row IDs. I'll see about adding a test but I'm not sure if we really even expose the mechanisms to produce a V2 manifest with first row IDs.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221862106
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
+ table.updateProperties().set(TableProperties.MANIFEST_MIN_MERGE_COUNT, "1").commit();
+ table.newRewrite().deleteFile(FILE_A).addFile(FILE_C).commit();
Review Comment:
I just realized that combining the operation may be required because we need a manifest that doesn't have a `first_row_id` assigned yet. If that's the case then there should be a comment that these need to happen in the same operation.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221858328
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
Review Comment:
We ran into this in the past as well, If I understand this correctly, this force trigger the manifest merge at the snapshot commit time wher ManifestMergeManager kick in to reads this manifest to merge
it with existing manifests by constructs a `ManifestReader` with `firstRowId=null`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222223572
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
Review Comment:
Yeah, I've gone through and cleaned all these up,
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
+ table.updateProperties().set(TableProperties.MANIFEST_MIN_MERGE_COUNT, "1").commit();
+ table.newRewrite().deleteFile(FILE_A).addFile(FILE_C).commit();
Review Comment:
Added some more details to the 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222643179
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
Since I think we have a reliable mechanism to distinguish commited vs uncommited manifests , I left out Preconditions checks for now just to keep things simple. Let me know if people feel strongly about this. I think whatever expectations we know we have on the entry at different points probably should have a Precondition check but at the same time I don't want to be over specific on our checks and cause needless failures when reading these things.
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
Since I think we have a reliable mechanism to distinguish commited vs uncommited manifests , I left out Preconditions checks for now just to keep things simple. Let me know if people feel strongly about this. I think whatever expectations we know we have on the entry at different points probably should have a Precondition check but at the same time I don't want to be over specific on our checks and cause needless failures when reading the manifests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221794518
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
Review Comment:
Robot saw the unicode arrows here, we should probably remove them and stick to ascii
```suggestion
// FILE_A->0, FILE_B->125; nextRowId=225
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222247033
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
Ok, I think I get the rationale for the defensive null assignment now. Preserving the entry made the upgrade test cases work because those will anyways have null first row IDs but this is to be defensive against someone producing older manifests with a first row ID somehow?
In either case, I've updated to pass in a committed flag to a pakcage private ManifestFiles.read API , and if it's true and firstRowId is null we do the null assignment, and if not, we preserve the entry as it is.
@RussellSpitzer I think you're saying we should add a Preconditions check for the committed case that the firstRowId is indeed null. I can get behind that, that seemed to be what @rdblue was implying in our offline conversation, if that's our expectation we should have a precondition check.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #16263:
URL: https://github.com/apache/iceberg/pull/16263
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3213456577
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,8 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
Review Comment:
I'm still checking if changing idAssigner like this for all cases is really the right way to fix this or if during manifest merging we should actually pass in our own id assignment transformer that preserves the existing case.
Here we're changing the manifest reader because
```
// data file's first_row_id is null when the manifest's first_row_id is null
```
isn't true in the context of reading merged manifests that have EXISTING entries which need to have first row IDs preserved but the merged manifest does not have a first row ID.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3214307973
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,8 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
Review Comment:
The global change reads correctly. The `else` branch only runs when `manifest.firstRowId` is null, and in every such case the on-disk `file.firstRowId` is what should be preserved:
- pre-v3 manifests: `file.firstRowId` is null in the data, identity preserves null → unchanged behavior
- v3 EXISTING entries in a transient filtered/merged manifest: identity preserves the original assignment → the fix
- v3 ADDED entries in a freshly-written manifest: `file.firstRowId` is null in the data (assigned at read time once the manifest gets a `firstRowId`), identity preserves null → unchanged behavior
I am also wondering about the reason for [the previous behavior change](https://github.com/apache/iceberg/pull/12843/changes#diff-79baa98562948784b4c255ee2dd14ae3a26f875d246f1607f76d58668285ef31). Maybe it is a defensive check covering: `ManifestFiles.copyAppendManifest`, the user-facing `appendManifest()` flow. A user can construct a `ManifestFile` externally with a non-null file-level `first_row_id` set on an ADDED entry — nothing in the public API stops that.
Under the previous reader-side clobber, the value was stripped before the writer saw it. Under `Function.identity()`, the value reaches the writer — but `writer.add(entry)` → `wrapAppend` → `Delegates.suppressFirstRowId`, so the wrapper's `firstRowId()` returns null and the bytes written for the ADDED entry have null on disk. Identical on-disk result.
The reader-side clobber was redundant on this path; the writer-side suppression in `MergingSnapshotProducer.add` and `GenericManifestEntry.wrapAppend` carries the "no override from client" enforcement. The global change preserves that property.
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,8 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // preserve any existing file-level first_row_id
Review Comment:
Suggested wording that names the specific case the branch handles:
```suggestion
// preserve any existing file-level first_row_id. EXISTING entries carried over
// from a rewrite or merge. Keep their original first_row_id even when the containing
// manifest hasn't been assigned its own first_row_id yet.
```
A future reader sees exactly which call path is defended — the merge-of-just-written-filtered-manifest case where the previous null-clobber lost row IDs.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3213456577
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,8 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
Review Comment:
I'm still checking if changing idAssigner like this for all cases is really the right way to fix this or if during manifest merging we should actually pass in our own id assignment transformer that preserves the existing case.
Here we're changing the manifest reader because
```
// data file's first_row_id is null when the manifest's first_row_id is null
```
isn't true in the context of reading a newly merged manifest. The manfiest first row ID is null, but the merged content could contain an existing entry with an already assigned first row ID.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3213939933
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +782,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
+ table.updateProperties().set(TableProperties.MANIFEST_MIN_MERGE_COUNT, "1").commit();
+ table.newRewrite().rewriteFiles(Set.of(FILE_A), Set.of(FILE_C)).commit();
Review Comment:
```suggestion
table.newRewrite().deleteFile(FILE_A).addFile(FILE_C).commit();
```
nit: `rewriteFiles` is deprecated
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3228796979
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -192,7 +196,9 @@ private ManifestFile createManifest(int specId, List<ManifestFile> bin) {
boolean threw = true;
try {
for (ManifestFile manifest : bin) {
- try (ManifestReader<F> reader = newManifestReader(manifest)) {
+ boolean isCommitted =
+ manifest.snapshotId() != null && snapshotId() != manifest.snapshotId();
Review Comment:
This looks reasonable to me.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3228187979
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,33 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
Review Comment:
Yes but, if we think it's important to handle that use case we should be testing it imho
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3220306948
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -252,7 +252,9 @@ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
return CloseableIterable.filter(
content == FileType.DATA_FILES
- ? scanMetrics.skippedDataFiles()
+ ? scanMetrics
+ .skippedDataFiles() // from a rewrite or merge. Keep their original first_row_id
Review Comment:
typo...
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222034887
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
Review Comment:
Smart qoutes (Unicode) in the comment, Robot saw these. Change to Ascii
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3226948413
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
I do, but I think I'm currently overruled by the majority. I don't like it when we change objects to "fix" them
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222247033
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
Ok, I think I get the rationale for the defensive null assignment now. Preserving the entry made the upgrade test cases work because those will anyways have null first row IDs but this is to be defensive against someone producing older manifests with a first row ID somehow?
In either case, I've updated to pass in a committed flag to a pakcage private ManifestFiles.read API , and if it's true we do the null assignment, and if not, we preserve the entry as it is.
@RussellSpitzer I think you're saying we should add a Preconditions check for the committed case that the firstRowId is indeed null. I can get behind that, that seemed to be what @rdblue was implying in our offline conversation, if that's our expectation we should have a precondition check.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3220256497
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,8 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
Review Comment:
Yeah @stevenzwu that's my analysis of all the call points as well. Fundamentally, I would expect the idTransformer that's set by default on the ManifestReader should do *no* transformation of the first row ID on the entry. That implicitly covers the 3 cases you mentioned.
And for the coppyAppendManifest case, if we really want to be defensive, I think the right solution is that the public writer APIs Iceberg exposes (that produce the manifests) should suppress the firstRowId assignment on the ADDED entry. The read side should just consume what's there in my opinion.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3227710479
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,33 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
Review Comment:
While we are at it we should add in a test for the "set row_id" to null assignment we do. The fact that we could change the behavior (in your previous commit) and not fail anything in the test suite is an issue. Especially if we think that is an important behavior.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3214266603
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,8 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // preserve any existing file-level first_row_id
Review Comment:
Maybe add a more explanatory comment here?
```suggestion
// We must preserve existing row id assignments. Existing entries
// carry their own file-level first_row_id from the source manifest,
// even though intermediate manifests may have a null first_row_id.
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#issuecomment-4423349220
needs to rebase https://github.com/apache/iceberg/pull/16287 to unblock the "Kafka Connect CVE Scan" issue in CI
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3227948840
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,33 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
Review Comment:
So there's a nuance to "change the behavior" which is why all the tests in the previous commit passed. Even with the previous commit, we ended up correctly setting row ID to null because the entries in older manifest wouldn't have a first row ID, so the behavior was preserved for the main case we care about for older manifests, and there's quite a few tests there for the upgrade path.
The case we're talking about though is a case where we (somehow?) produce a pre-v3 manifest with first row IDs and we should ignore those by defensively setting those to null; in that specific case, there was a behavior change because we'd just keep those row IDs. I'll see about adding a test but I'm not sure if we really even expose the mechanisms to produce a V2 manifest with first row IDs.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3228824164
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -1250,7 +1250,13 @@ protected ManifestWriter<DataFile> newManifestWriter(PartitionSpec manifestSpec)
@Override
protected ManifestReader<DataFile> newManifestReader(ManifestFile manifest) {
- return MergingSnapshotProducer.this.newManifestReader(manifest);
+ return newManifestReader(manifest, true);
Review Comment:
I'm fine with this default, but we should probably start requiring `isCommitted` to be passed. Can we deprecate the old one and start using this exclusively?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221830271
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
Review Comment:
Let's remove arrow characters. They're hard to read, at least in github.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3221858328
##########
core/src/test/java/org/apache/iceberg/TestRowLineageAssignment.java:
##########
@@ -781,6 +781,22 @@ public void testUpgradeAssignmentWithManifestCompaction(@TempDir File altLocatio
FILE_C.recordCount() + FILE_B.recordCount());
}
+ @Test
+ public void testRewritePreservesExistingFileFirstRowIds() {
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+ // FILE_A→0, FILE_B→125; nextRowId=225
Review Comment:
We ran into this in the past as well, If I understand this correctly, this force trigger the manifest merge at the snapshot commit time where ManifestMergeManager kick in to reads this manifest to merge
it with existing manifests by constructs a `ManifestReader` with `firstRowId=null`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222213526
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
+1 on two assigners.
I agree we should preserve the defensive mode for the committed-manifest path, but I don't think it should be a silent null assignment. If the invariant is "a committed v3 manifest with firstRowId != null shouldn't contain an entry with a non-null first_row_id that wasn't already there," we should encode that as a precondition / checkState and fail loudly. Silent re-assignment masks exactly the class of bug we're fixing here. If there's a real case where re-assigning is necessary, that seems like it should be opt-in behind a config flag rather than the default.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix row ID assignment for EXISTING entry during a manifest merge [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#issuecomment-4434751204
Ok I'll go ahead and merge. There are some followup items but I think the PR is in a good state for unblocking the release. Thanks everyone! @RussellSpitzer @stevenzwu @rdblue @mxm @dramaticlly @aihuaxu @kevinjqliu for reviewing
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org