You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nk1506 (via GitHub)" <gi...@apache.org> on 2024/04/29 09:46:15 UTC
[PR] Add ManifestFile Stats in snapshot summary. [iceberg]
nk1506 opened a new pull request, #10246:
URL: https://github.com/apache/iceberg/pull/10246
Currently snapshot summary doesn't have statistics related to Manifest Files.
This change is adding two new summary fields `"total-data-manifest-files"` and `"total-delete-manifest-files"`. There is a plan to help engines to do better planning estimation in terms of manifest reading.
Old Snapshot Summary
```
{
"added-data-files": "1",
"added-files-size": "389",
"added-records": "1",
"changed-partition-count": "1",
"spark.app.id": "local-1714372186602",
"total-data-files": "1",
"total-delete-files": "0",
"total-equality-deletes": "0",
"total-files-size": "389",
"total-position-deletes": "0",
"total-records": "1"
}
```
New Snapshot Summary
```
{
"added-data-files": "1",
"added-files-size": "389",
"added-records": "1",
"changed-partition-count": "1",
"spark.app.id": "local-1714372186602",
"total-data-files": "1",
"total-delete-files": "0",
"total-equality-deletes": "0",
"total-files-size": "389",
"total-position-deletes": "0",
"total-records": "1",
"total-data-manifest-files": "1",
"total-delete-manifest-files": "1"
}
```
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1607253850
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
cc @electrum
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1589716377
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
> But definitely open to hearing more, and especially if you are able to share any data points on how this metric helped ; that would give us some more confidence that this is useful and can generalize.
+1 on this, how would knowing these 2 new stats help with planning?
- Knowing how many can help you skip any of the manifests during planning?
- or knowing such will help make manifest rewrite decision?
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#issuecomment-2451055953
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1584982185
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
This is also my main question.
My train of thought: You will need to read the manifest-list in any situation. The number of manifest can vary widely:
- If FastAppends are used frequently, there will be many small manifests that you want to bundle into batches.
- If MergeAppends are used, the manifests are rather hefty (8 megabytes by default, set using `commit.manifest.target-size-bytes`).
With the knowledge from the summary, you could spin up executors before reading the manifest-list, but this can be difficult since you would also need to know the sizes of the manifest to do some effective planning.
The downside is that we add extra information to the metadata-JSON, which can also grow in size when there are many commits.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1713644175
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
@ajantha-bhat I'm still thinking the argument of having this information helping the query planning is quite thin. I don't think you can get away with reading the manifest list for doing some meaningful query planning as the size of the manifests varies wildly. Thinking of it, another issue can be that the manifest is not live, meaning it only contains deleted manifest-entries in a certain manifest file. You'll get all this information when you read the manifest list.
> Agree that Having size based cost estimation will be more accurate. But count based estimation is still better than no stats.
Everything comes at a price. The snapshots are already a substantial portion of the table metadata, and users are already running into issues when the number of snapshots becomes too large.
Looping in @aokolnychyi in here as well to get his opinion since he did a lot of work on performance optimization
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#issuecomment-2465928359
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1584935556
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
Stupid question: do we have to rough estimate on this change in terms of manifest append performance ? I guess it's not important, but just wondering.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1594265875
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
I had a discussion with @nk1506 to understand it better.
Dremio (or query engines that uses CBO) need to estimate the cost of the query plan.
parallelism is one of the factor for cost estimation.
Query has planning and execution phase.
So, during planning phase we would like to know how many manifests exist to estimate the parallelism required for reading manifests without doing an actual IO of manifest list (as we want planning phase to be as fast as possible).
We currently estimate the parallelism of data files IO by reading the stats that exist in the snapshot summary. But the stats related to manifest count is missing in snapshot summary and we are unable to estimate. Hence, the 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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #10246: Add ManifestFile Stats in snapshot summary.
URL: https://github.com/apache/iceberg/pull/10246
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1584137262
##########
core/src/main/java/org/apache/iceberg/SnapshotSummary.java:
##########
@@ -263,6 +273,12 @@ void addTo(ImmutableMap.Builder<String, String> builder) {
setIf(removedDeleteFiles > 0, builder, REMOVED_DELETE_FILES_PROP, removedDeleteFiles);
setIf(addedRecords > 0, builder, ADDED_RECORDS_PROP, addedRecords);
setIf(deletedRecords > 0, builder, DELETED_RECORDS_PROP, deletedRecords);
+ setIf(totalDataManifestFiles > 0, builder, TOTAL_DATA_MANIFEST_FILES, totalDataManifestFiles);
Review Comment:
If there are zero counts these stats will be not added to summary.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1585147537
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
> Stupid question: do we have a rough estimate on this change in terms of manifest append performance ? I guess it's not important, but just wondering.
_It is only going to add cost with metadata json size._ _Adding it to snapshot summary is inline 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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1589368924
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
Generally speaking, I'm in favor of adding metrics in manifest list if it helps engines during planification.
I don't see how `total-data-manifest-files` and `total-delete-manifest-files` would be helpful for the planner, but you know better than me here ๐
I would understand straight if the planner would have actually the location of the delete manifest files, as he would be able to "bypass" reading them. So maybe this PR is the first on a series where the next ones would add concrete location. Just thinking loud again.
To summarize: ok with this change generally speaking even if I don't see how it would help the planner ๐
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1584935556
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
Stupid question: do we have a rough estimate on this change in terms of manifest append performance ? I guess it's not important, but just wondering.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1589621767
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
@amogh-jahagirdar , I agree we should also consider Manifest Size for better estimation. But don't you think we can solve this problem by [rewrite_manifests](https://iceberg.apache.org/docs/latest/spark-procedures/#rewrite_manifests).
For any engine if it is running compaction first and then doing GC. Do you think Manifest size(say every file is of ~8MB) will be relevant ?
Please share your thoughts.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1585139953
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
Thanks @Fokko for feedback.
> With the knowledge from the summary, you could spin up executors before reading the manifest-list, but this can be difficult since you would also need to know the sizes of the manifest to do some effective planning.
_With manifest stats, It can help with better planning in terms of manifest file scans. Without these stats in metadata json either someone will have to assume or do one more IO with manifestList._
> The downside is that we add extra information to the metadata-JSON, which can also grow in size when there are many commits.
_I agree it adds some extra bytes to metadataJson. But I think here we have options to optimize with expire snapshots. But to avoid an extra IO for planning I dont find any other way._
WDYT ?
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1713869763
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
> I'm still thinking the argument of having this information helping the query planning is quite thin
Thanks. We will try out this change in our fork and benchmark the results with and without this change for our engine (Dremio). So, we will have a clear understanding of the benefits of these changes.
The other engines that has CBO like Trino can also try it out.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1594277743
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java:
##########
@@ -180,8 +181,10 @@ public void testBinPackUnpartitionedTable() {
assertThat(result.rewrittenBytesCount()).isEqualTo(dataSizeBefore);
shouldHaveFiles(table, 1);
- List<Object[]> actual = currentData();
+ testManifestStats(table);
Review Comment:
We can test these at the end of the test also to avoid multiple line diff.
Similarly for all the places.
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
ping @Fokko, @amogh-jahagirdar, @dramaticlly: Can we conclude (move forward) on this based on my above comment? Having these stats will leads to better estimate based on parallelism. Agree that Having size based cost estimation will be more accurate. But count based estimation is still better than no stats.
Is other engines that use CBO like Trino is intersted in these stats?
cc: @findepi, @findinpath thoughts?
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSnapshotsTable.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark.sql;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.MetadataTableType;
+import org.apache.iceberg.spark.CatalogTestBase;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+
+public class TestSnapshotsTable extends CatalogTestBase {
+
+ @BeforeEach
+ public void createTables() {
+ sql(
+ "CREATE TABLE %s (id int, data string) USING iceberg "
+ + "TBLPROPERTIES"
+ + "('format-version'='2',"
+ + "'write.delete.mode'='merge-on-read')",
+ tableName);
+ sql("INSERT INTO %s VALUES (1, 'a1'),(2, 'a2'),(3, 'a3')", tableName);
+ }
+
+ @AfterEach
+ public void removeTables() {
+ sql("DROP TABLE IF EXISTS %s", tableName);
+ }
+
+ @TestTemplate
+ public void testSnapshotsTable() {
+ List<Object[]> sql = sql("SELECT * FROM %s.%s", tableName, MetadataTableType.SNAPSHOTS);
+ assertThat(sql).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testTotalDataManifestFilesWithSnapshotsTableSummary() {
+ List<Object[]> sql = sql("SELECT * FROM %s.%s", tableName, MetadataTableType.SNAPSHOTS);
+ assertThat(sql).hasSize(1);
+ Map<String, String> summary = (Map) sql.get(0)[5];
+ assertThat(summary.get("total-data-manifest-files")).isEqualTo("1");
+ assertThat(summary.get("total-delete-manifest-files")).isEqualTo(null);
+ sql("INSERT INTO %s VALUES (4, 'a4')", tableName);
+ sql = sql("SELECT * FROM %s.%s", tableName, MetadataTableType.SNAPSHOTS);
+ assertThat(sql).hasSize(2);
+ summary = (Map) sql.get(1)[5];
+ assertThat(summary.get("total-data-manifest-files")).isEqualTo("2");
+ assertThat(summary.get("total-delete-manifest-files")).isEqualTo(null);
Review Comment:
```suggestion
assertThat(summary.get("total-delete-manifest-files"))..isNull();
```
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSnapshotsTable.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark.sql;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.MetadataTableType;
+import org.apache.iceberg.spark.CatalogTestBase;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+
+public class TestSnapshotsTable extends CatalogTestBase {
+
+ @BeforeEach
+ public void createTables() {
+ sql(
+ "CREATE TABLE %s (id int, data string) USING iceberg "
+ + "TBLPROPERTIES"
+ + "('format-version'='2',"
+ + "'write.delete.mode'='merge-on-read')",
+ tableName);
+ sql("INSERT INTO %s VALUES (1, 'a1'),(2, 'a2'),(3, 'a3')", tableName);
+ }
+
+ @AfterEach
+ public void removeTables() {
+ sql("DROP TABLE IF EXISTS %s", tableName);
+ }
+
+ @TestTemplate
+ public void testSnapshotsTable() {
+ List<Object[]> sql = sql("SELECT * FROM %s.%s", tableName, MetadataTableType.SNAPSHOTS);
+ assertThat(sql).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testTotalDataManifestFilesWithSnapshotsTableSummary() {
+ List<Object[]> sql = sql("SELECT * FROM %s.%s", tableName, MetadataTableType.SNAPSHOTS);
+ assertThat(sql).hasSize(1);
+ Map<String, String> summary = (Map) sql.get(0)[5];
+ assertThat(summary.get("total-data-manifest-files")).isEqualTo("1");
+ assertThat(summary.get("total-delete-manifest-files")).isEqualTo(null);
+ sql("INSERT INTO %s VALUES (4, 'a4')", tableName);
+ sql = sql("SELECT * FROM %s.%s", tableName, MetadataTableType.SNAPSHOTS);
+ assertThat(sql).hasSize(2);
+ summary = (Map) sql.get(1)[5];
+ assertThat(summary.get("total-data-manifest-files")).isEqualTo("2");
+ assertThat(summary.get("total-delete-manifest-files")).isEqualTo(null);
Review Comment:
similarly check and replace all the places.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1584136686
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -190,6 +190,7 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
List<ManifestFile> apply = Lists.newArrayList();
Iterables.addAll(apply, newManifestsWithMetadata);
apply.addAll(keptManifests);
+ apply.forEach(summaryBuilder::addedManifestStats);
Review Comment:
Add manifestStats with any newManifest.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1584982185
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
This is also my main question.
My train of thought: You will need to read the manifest-list in any situation. The number of manifest can vary widely:
- If FastAppends are used frequently, there will be many small manifests that you want to bundle into batches.
- If MergeAppends are used, the manifests are rather hefty (8 megabytes by default, set using `commit.manifest.target-size-bytes`).
With the knowledge from the summary, you could spin up executors prior reading the manifest-list, but this can be difficult since you would also need to know the sizes of the manifest to do some effective planning.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1592861026
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
Thanks for the feedback.
Regarding the usages of manifest counts for planning here is my feedback:
1. Having Manifest counts in advance helps to plan the parallelism. Like [spark](https://github.com/apache/iceberg/blob/ed0959257cba02f378f7097d81cecaaaef9fa43f/core/src/main/java/org/apache/iceberg/BaseDistributedDataScan.java#L149) is doing after reading from ManifestList.
2. How it will help with SnapshotSummary ?
> Engine like Spark doesn't get any benefits from these stats. Since it's parallelism is dynamic with runtime in nature.
> But other engines like Dremio which decides it's parallelism(during compiletime) in advance. Providing these stats will help for better parallelism.
--
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] Add ManifestFile Stats in snapshot summary. [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10246:
URL: https://github.com/apache/iceberg/pull/10246#discussion_r1588511039
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -156,6 +156,8 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
manifests.addAll(snapshot.allManifests(ops.io()));
}
+ manifests.forEach(summaryBuilder::addedManifestStats);
Review Comment:
@nk1506 I think the main question (at least from me) is essentially:
Is information on just the total number of data or delete manifests actually helpful for being able to help with whatever planning estimation you're trying to do?
As @Fokko said you'll probably want to know manifest sizes (in bytes) of the files involved in planning since there can be variance depending on the strategy used for the append. I think with just determining based on number of files (and not including sizes, which means you have to read the manifest anyways) there would be a lot of overprovisioning and underprovisioning (if you're trying to a distributed planning).
But definitely open to hearing more, and especially if you are able to share any data points on how this metric helped ; that would give us some more confidence that this is useful and can generalize.
--
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