You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2023/10/27 01:17:14 UTC
[PR] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
aokolnychyi opened a new pull request, #8928:
URL: https://github.com/apache/iceberg/pull/8928
This PR avoids extra copies of manifests while optimizing metadata for V2 tables via Spark. Specifically, we have to rewrite all remotely produced manifests while committing again to assign explicit snapshot IDs in V1 tables unless the snapshot ID inheritance is enabled (false by default). In V2 tables, however, snapshot ID inheritance is always enabled. That's why it is safe to reuse the remotely produced manifests without an expensive rewrite on the driver as long as we have a V2 table.
--
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] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #8928:
URL: https://github.com/apache/iceberg/pull/8928#discussion_r1376706853
##########
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteManifestsSparkAction.java:
##########
@@ -337,7 +337,7 @@ private void replaceManifests(
addedManifests.forEach(rewriteManifests::addManifest);
commit(rewriteManifests);
- if (!snapshotIdInheritanceEnabled) {
+ if (formatVersion == 1 && !snapshotIdInheritanceEnabled) {
Review Comment:
Minor suggestion: can we have format version as an extra parameter for the method as following, so that we can consolidate the logic from multiple places?
```
PropertyUtil.propertyAsBoolean(
table.properties(),
TableProperties.SNAPSHOT_ID_INHERITANCE_ENABLED,
TableProperties.SNAPSHOT_ID_INHERITANCE_ENABLED_DEFAULT,
formatVersion);
```
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -82,10 +83,12 @@ public class BaseRewriteManifests extends SnapshotProducer<RewriteManifests>
this.manifestTargetSizeBytes =
ops.current()
.propertyAsLong(MANIFEST_TARGET_SIZE_BYTES, MANIFEST_TARGET_SIZE_BYTES_DEFAULT);
- this.snapshotIdInheritanceEnabled =
+ this.formatVersion = ops.current().formatVersion();
+ boolean snapshotIdInheritanceEnabled =
ops.current()
.propertyAsBoolean(
SNAPSHOT_ID_INHERITANCE_ENABLED, SNAPSHOT_ID_INHERITANCE_ENABLED_DEFAULT);
+ this.canInheritSnapshotId = formatVersion > 1 || snapshotIdInheritanceEnabled;
Review Comment:
We may update the doc(https://iceberg.apache.org/docs/latest/configuration/#compatibility-flags) a bit to be clear that v2 table doesn't honor the property `compatibility.snapshot-id-inheritance.enabled`, in case people get confused. Not a blocker. We can do it in a follow up PR.
--
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] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8928:
URL: https://github.com/apache/iceberg/pull/8928#discussion_r1373976469
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -160,11 +163,10 @@ public RewriteManifests addManifest(ManifestFile manifest) {
}
private ManifestFile copyManifest(ManifestFile manifest) {
- TableMetadata current = ops.current();
InputFile toCopy = ops.io().newInputFile(manifest.path());
OutputFile newFile = newManifestOutput();
return ManifestFiles.copyRewriteManifest(
- current.formatVersion(),
+ formatVersion,
Review Comment:
Need to compute the format version earlier so simply reuse it here.
--
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] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8928:
URL: https://github.com/apache/iceberg/pull/8928#discussion_r1377994540
##########
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteManifestsSparkAction.java:
##########
@@ -337,7 +337,7 @@ private void replaceManifests(
addedManifests.forEach(rewriteManifests::addManifest);
commit(rewriteManifests);
- if (!snapshotIdInheritanceEnabled) {
+ if (formatVersion == 1 && !snapshotIdInheritanceEnabled) {
Review Comment:
I tried to generalize this in `SnapshotProducer`, I'd delay a utility call like this until we have multiple use cases. It is a bit special as if format is V2, the value must be ignored.
--
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] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8928:
URL: https://github.com/apache/iceberg/pull/8928#discussion_r1373976469
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -160,11 +163,10 @@ public RewriteManifests addManifest(ManifestFile manifest) {
}
private ManifestFile copyManifest(ManifestFile manifest) {
- TableMetadata current = ops.current();
InputFile toCopy = ops.io().newInputFile(manifest.path());
OutputFile newFile = newManifestOutput();
return ManifestFiles.copyRewriteManifest(
- current.formatVersion(),
+ formatVersion,
Review Comment:
Need to compute the format version earlier so simply reuse it here.
--
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] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8928:
URL: https://github.com/apache/iceberg/pull/8928#discussion_r1374826073
##########
core/src/test/java/org/apache/iceberg/TestRewriteManifests.java:
##########
@@ -443,6 +444,14 @@ public void testBasicManifestReplacement() throws IOException {
List<ManifestFile> manifests = snapshot.allManifests(table.io());
Assert.assertEquals(3, manifests.size());
+ if (formatVersion == 1) {
+ assertThat(manifests.get(0).path()).isNotEqualTo(firstNewManifest.path());
+ assertThat(manifests.get(1).path()).isNotEqualTo(secondNewManifest.path());
+ } else {
+ assertThat(manifests.get(0).path()).isEqualTo(firstNewManifest.path());
+ assertThat(manifests.get(1).path()).isEqualTo(secondNewManifest.path());
+ }
Review Comment:
There are actually two different tests. This one called basicReplacement and tests the default behavior. The one below explicitly validates enabled snapshot ID inheritance.
--
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] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #8928:
URL: https://github.com/apache/iceberg/pull/8928#discussion_r1374737812
##########
core/src/test/java/org/apache/iceberg/TestRewriteManifests.java:
##########
@@ -443,6 +444,14 @@ public void testBasicManifestReplacement() throws IOException {
List<ManifestFile> manifests = snapshot.allManifests(table.io());
Assert.assertEquals(3, manifests.size());
+ if (formatVersion == 1) {
+ assertThat(manifests.get(0).path()).isNotEqualTo(firstNewManifest.path());
+ assertThat(manifests.get(1).path()).isNotEqualTo(secondNewManifest.path());
+ } else {
+ assertThat(manifests.get(0).path()).isEqualTo(firstNewManifest.path());
+ assertThat(manifests.get(1).path()).isEqualTo(secondNewManifest.path());
+ }
Review Comment:
[minor] A comment might be helpful
shouldn't we check if it's format = 1 and snapshotIdInheritence enabled ?
##########
core/src/test/java/org/apache/iceberg/TestRewriteManifests.java:
##########
@@ -443,6 +444,14 @@ public void testBasicManifestReplacement() throws IOException {
List<ManifestFile> manifests = snapshot.allManifests(table.io());
Assert.assertEquals(3, manifests.size());
+ if (formatVersion == 1) {
+ assertThat(manifests.get(0).path()).isNotEqualTo(firstNewManifest.path());
+ assertThat(manifests.get(1).path()).isNotEqualTo(secondNewManifest.path());
+ } else {
+ assertThat(manifests.get(0).path()).isEqualTo(firstNewManifest.path());
+ assertThat(manifests.get(1).path()).isEqualTo(secondNewManifest.path());
+ }
Review Comment:
[minor] shouldn't we check if it's format = 1 and snapshotIdInheritence enabled ?
--
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] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8928:
URL: https://github.com/apache/iceberg/pull/8928#discussion_r1377066516
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -82,10 +83,12 @@ public class BaseRewriteManifests extends SnapshotProducer<RewriteManifests>
this.manifestTargetSizeBytes =
ops.current()
.propertyAsLong(MANIFEST_TARGET_SIZE_BYTES, MANIFEST_TARGET_SIZE_BYTES_DEFAULT);
- this.snapshotIdInheritanceEnabled =
+ this.formatVersion = ops.current().formatVersion();
+ boolean snapshotIdInheritanceEnabled =
ops.current()
.propertyAsBoolean(
SNAPSHOT_ID_INHERITANCE_ENABLED, SNAPSHOT_ID_INHERITANCE_ENABLED_DEFAULT);
+ this.canInheritSnapshotId = formatVersion > 1 || snapshotIdInheritanceEnabled;
Review Comment:
Updated the doc in this PR, that's a good point.
--
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] Spark: Avoid extra copies of manifests while optimizing V2 tables [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #8928:
URL: https://github.com/apache/iceberg/pull/8928
--
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