You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "bknbkn (via GitHub)" <gi...@apache.org> on 2024/01/09 07:48:04 UTC
[PR] Spark 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
bknbkn opened a new pull request, #9447:
URL: https://github.com/apache/iceberg/pull/9447
This patch add `where` args in `RewritingManifestsProcedure`, It allows us to only rewrite the manifest file of the specified conditions. When we optimize the manifests of a large table, rewriting the manifests of the entire table will cost time, sometimes we only want to rewrite the manifests of the most recent partitions, then specifying the filter for rewriting manifests procedure will be helpful.
Like `RewritingDatafilesProcedure`, `where` args is predicate as a string used for filtering the manifests. All manifests that may contain entry matching the filter will be selected for rewriting (the filter will be always true if it don't contain any partition field)
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "bknbkn (via GitHub)" <gi...@apache.org>.
bknbkn commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1458505983
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
In our scenario, `Maintenance Action` more often exists as a periodic task, we will automatically call `Maintenance Action` regularly to maintain many existing tables in the warehouse, most of tables are time-partitioned tables. Adding filter brings the following benefits:
- **Reduce unnecessary manifests IO in the cluster**
Within an action execution interval, only a part of the partition data of each table will be updated (in our scenario about 10% of partitions will be affected, the manifests contained remaining partitions have been aggregated when the previous action was executed, and no need to re-aggregate). For a single table, it will not take more than **10 minutes** to execute rewrite manifests in a distributed situation, but we still hope to have the opportunity to reduce the overall pressure on the cluster.
- **Conflict avoidance**
When too many small files are written to the upstream of the Iceberg table at one time, it may trigger manfests merging(By ManifestMergeManager.mergeGroup). This behavior may conflict with the rewrite manifests action. Facing streaming tasks, we cannot stop it, can only be solved by retrying the action. But filter out the partition the task is currently writing can reduce the occurrence of conflicts.
**By the way,** In V1 table with snapshot-id inherit=true, it still need write all manifests in driver when execute copyManifests function, I find that this propertity is no longer affects V2 table, Is any plan to remove or default true this propertity in the V1 table, or is there still any unsafe?
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "bknbkn (via GitHub)" <gi...@apache.org>.
bknbkn commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1470704554
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
Got it, thanks for your answer @aokolnychyi !
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1468199919
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
I agree that there are some cases where it could be beneficial, but like you said, it would only work in some special cases (e.g. time-based partitioning and writing data only to new partitions and never to old ones). However, I am not sure how generic this is. If we add this new argument, it may not work in a lot of cases, causing confusion.
What do you think about using the underlying action directly for your needs? There, you can pass a closure and have sophisticated filtering logic even beyond partition bounds (e.g. you can check snapshot IDs).
> By the way, In V1 table with snapshot-id inherit=true, it still need write all manifests in driver when execute copyManifests function, I find that this propertity is no longer affects V2 table, Is any plan to remove or default true this propertity in the V1 table, or is there still any unsafe?
I highly recommend enabling that property in all tables if you know that it will not brake some old writers/readers. We can't make that guarantee on behalf of the user.
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "bknbkn (via GitHub)" <gi...@apache.org>.
bknbkn commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1458505983
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
Thanks for your review @aokolnychyi , In our scenario, `Maintenance Action` more often exists as a periodic task, we will automatically call `Maintenance Action` regularly to maintain many existing tables in the warehouse, most of tables are time-partitioned tables. Adding filter brings the following benefits:
- **Reduce unnecessary manifests IO in the cluster**
Within an action execution interval, only a part of the partitions data of each table will be updated (in our scenario about 10% of partitions will be affected, the manifests contained remaining partitions have been aggregated when the previous action was executed, and no need to re-aggregate). For a single table, rewrite all manifests will not take more than **10 minutes** in a distributed situation, but we still hope to have the opportunity to reduce the overall pressure on the cluster.
- **Conflict avoidance**
When too many small files are written to the upstream of the Iceberg table at one time, it may trigger manfests merging(By ManifestMergeManager.mergeGroup). This behavior may conflict with the rewrite manifests action. Facing streaming tasks, we cannot stop it, can only be solved by retrying the action. But filter out the partition the task is currently writing can reduce the occurrence of conflicts.
**By the way,** In V1 table with snapshot-id inherit=true, it still need write all manifests in driver when execute copyManifests function, I find that this propertity is no longer affects V2 table, Is any plan to remove or default true this propertity in the V1 table, or is there still any unsafe?
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1474296939
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
time-based partition table is not merely some special cases in our practice. in fact they are the majority 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: 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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "bknbkn (via GitHub)" <gi...@apache.org>.
bknbkn commented on PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#issuecomment-1884154630
cc @aokolnychyi, would you mind to review this since you completed most parts of the rewrite manifests actions. Thanks a lot.
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#issuecomment-1894364112
Will review this week, thanks for pinging, @bknbkn!
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "bknbkn (via GitHub)" <gi...@apache.org>.
bknbkn commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1458505983
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
In our scenario, `Maintenance Action` more often exists as a periodic task, we will automatically call `Maintenance Action` regularly to maintain many existing tables in the warehouse, most of tables are time-partitioned tables. Adding filter brings the following benefits:
- **Reduce unnecessary manifests IO in the cluster**
Within an action execution interval, only a part of the partition data of each table will be updated (in our scenario about 10% of partitions will be affected, the manifests contained remaining partitions have been aggregated when the previous action was executed, and no need to re-aggregate). For a single table, it will not take more than **10 minutes** to execute rewrite all manifests in a distributed situation, but we still hope to have the opportunity to reduce the overall pressure on the cluster.
- **Conflict avoidance**
When too many small files are written to the upstream of the Iceberg table at one time, it may trigger manfests merging(By ManifestMergeManager.mergeGroup). This behavior may conflict with the rewrite manifests action. Facing streaming tasks, we cannot stop it, can only be solved by retrying the action. But filter out the partition the task is currently writing can reduce the occurrence of conflicts.
**By the way,** In V1 table with snapshot-id inherit=true, it still need write all manifests in driver when execute copyManifests function, I find that this propertity is no longer affects V2 table, Is any plan to remove or default true this propertity in the V1 table, or is there still any unsafe?
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#issuecomment-2408754895
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1457935454
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
The reason why we did not add it in the first place is because we persist only lower/upper boundaries for manifest partition values. Therefore, filters may not necessarily benefit from that. Based on what I've seen, these boundaries are rarely helpful when the manifest compaction is needed. Also, we have done some improvements to how the manifests are rewritten (not released yet). From what I see, it is a matter of a few minutes to rewrite all manifests even for large tables because the operation is distributed.
@bknbkn, did you have a chance to try the improvements for the action in master? How long does the rewrite take in your tables if you target all 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] Spark 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "bknbkn (via GitHub)" <gi...@apache.org>.
bknbkn commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1458505983
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
In our scenario, `Maintenance Action` more often exists as a periodic task, we will automatically call `Maintenance Action` regularly to maintain many existing tables in the warehouse, most of tables are time-partitioned tables. Adding filter brings the following benefits:
- **Reduce unnecessary manifests IO in the cluster**
Within an action execution interval, only a part of the partition data of each table will be updated (in our scenario about 10% of partitions will be affected, the manifests contained remaining partitions have been aggregated when the previous action was executed, and no need to re-aggregate). For a single table, rewrite all manifests will not take more than **10 minutes** in a distributed situation, but we still hope to have the opportunity to reduce the overall pressure on the cluster.
- **Conflict avoidance**
When too many small files are written to the upstream of the Iceberg table at one time, it may trigger manfests merging(By ManifestMergeManager.mergeGroup). This behavior may conflict with the rewrite manifests action. Facing streaming tasks, we cannot stop it, can only be solved by retrying the action. But filter out the partition the task is currently writing can reduce the occurrence of conflicts.
**By the way,** In V1 table with snapshot-id inherit=true, it still need write all manifests in driver when execute copyManifests function, I find that this propertity is no longer affects V2 table, Is any plan to remove or default true this propertity in the V1 table, or is there still any unsafe?
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1474296939
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
time-based partition table is not merely some special cases in our practice. in fact they are the majority 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: 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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "bknbkn (via GitHub)" <gi...@apache.org>.
bknbkn commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1458505983
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
Thanks for your review @aokolnychyi , In our scenario, `Maintenance Action` more often exists as a periodic task, we will automatically call `Maintenance Action` regularly to maintain many existing tables in the warehouse, most of tables are time-partitioned tables. Adding filter brings the following benefits:
- **Reduce unnecessary manifests IO in the cluster**
Within an action execution interval, only a part of the partition data of each table will be updated (in our scenario about 10% of partitions will be affected, the manifests contained remaining partitions have been aggregated when the previous action was executed, and no need to re-aggregate). For a single table, rewrite all manifests will not take more than **10 minutes** in a distributed situation, but we still hope to have the opportunity to reduce the overall pressure on the cluster.
- **Conflict avoidance**
When too many small files are written to the upstream of the Iceberg table at one time, it may trigger manfests merging(By ManifestMergeManager.mergeGroup). This behavior may conflict with the rewrite manifests action. Facing streaming tasks, we cannot stop it, can only be solved by retrying the action. But filter out the partition the task is currently writing can reduce the occurrence of conflicts.
**By the way,** In V1 table with snapshot-id inherit=true, it still need write all manifests in driver when execute copyManifests function, I find that this propertity is no longer affects V2 table, Is any plan to remove or default true this propertity in the V1 table, or is there still any unsafe?
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#issuecomment-2424318521
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9447: Spark 3.5: Support specifying filter in RewriteManifestsProcedure
URL: https://github.com/apache/iceberg/pull/9447
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "bknbkn (via GitHub)" <gi...@apache.org>.
bknbkn commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1458505983
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
Thanks for your review @aokolnychyi , In our scenario, `Maintenance Action` more often exists as a periodic task, we will automatically call `Maintenance Action` regularly to maintain many existing tables in the warehouse, most of tables are time-partitioned tables. Adding filter brings the following benefits:
- **Reduce unnecessary manifests IO in the cluster**
Within an action execution interval, only a part of the partitions data of each table will be updated (in our scenario about 10% of partitions will be affected, the manifests contained remaining partitions have been aggregated when the previous action was executed, and no need to re-aggregate). For a single table, rewrite all manifests will not take more than **10 minutes** in a distributed situation, but we still hope to have the opportunity to reduce the overall pressure on the cluster.
- **Conflict avoidance**
When too many small files are written to the upstream of the Iceberg table at one time, it may trigger manfests merging(By ManifestMergeManager.mergeGroup). This behavior may conflict with the rewrite manifests action. Facing streaming tasks, we cannot stop it, can only be solved by retrying the action. But filter out the partition which the task is currently writing can reduce the occurrence of conflicts.
**By the way,** In V1 table with snapshot-id inherit=true, it still need write all manifests in driver when execute copyManifests function, I find that this propertity is no longer affects V2 table, Is any plan to remove or default true this propertity in the V1 table, or is there still any unsafe?
--
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 3.5: Support specifying filter in RewriteManifestsProcedure [iceberg]
Posted by "chenwyi2 (via GitHub)" <gi...@apache.org>.
chenwyi2 commented on code in PR #9447:
URL: https://github.com/apache/iceberg/pull/9447#discussion_r1590740653
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteManifestsProcedure.java:
##########
@@ -118,4 +126,15 @@ private InternalRow[] toOutputRows(RewriteManifests.Result result) {
public String description() {
return "RewriteManifestsProcedure";
}
+
+ private RewriteManifestsSparkAction checkAndApplyFilter(
+ RewriteManifestsSparkAction action, String where, Identifier ident) {
+ if (where != null) {
+ Expression expression = filterExpression(ident, where);
Review Comment:
> The reason why we did not add it in the first place is because we persist only lower/upper boundaries for manifest partition values. Therefore, filters may not necessarily benefit from that. Based on what I've seen, these boundaries are rarely helpful when the manifest compaction is needed. Also, we have done some improvements to how the manifests are rewritten (not released yet). From what I see, it is a matter of a few minutes to rewrite all manifests even for large tables because the operation is distributed.
>
> @bknbkn, did you have a chance to try the improvements for the action in master? How long does the rewrite take in your tables if you target all manifests?
could you please provide some pr which are improvements to how the manifests are rewritten?
We have some big tables which have too much manifest, and conflicts are always happening when rewriting manifest,
--
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