You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/09 18:57:21 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #5009: [WIP][CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
kbendick opened a new pull request, #5009:
URL: https://github.com/apache/iceberg/pull/5009
This closes issue https://github.com/apache/iceberg/issues/4689
Currently, if a table has the `WRITE_PARTITION_SUMMARY_LIMIT`, `write.summary.partition-limit`, set to a non-zero value, it will generate partition summary data for a partition field with an empty string name.
This field essentially comes from the unpartitioned spec.
Notice that `partitions.` field, which is the [changed partitions prefix](https://github.com/apache/iceberg/blob/3584c79022ec70f79b326550736b4600d249e4a2/core/src/main/java/org/apache/iceberg/SnapshotSummary.java#L54), should normally have the partition field name after it, as it's a summary for that partition field.
However, for unpartitioned tables, if the partition summary limit is greater than zero, a `partitions.` field will be added for an empty string column.
Here's an example:
```json
"summary" : {
"operation" : "append",
"spark.app.id" : "local-1651499999454",
"added-data-files" : "1",
"added-records" : "1",
"added-files-size" : "608",
"changed-partition-count" : "1",
"partition-summaries-included" : "true",
"partitions." : "added-data-files=1,added-records=1,added-files-size=608",
"total-records" : "1",
"total-files-size" : "608",
"total-data-files" : "1",
"total-delete-files" : "0",
"total-position-deletes" : "0",
"total-equality-deletes" : "0"
}
```
This PR removes partition summaries for commits on unpartitioned tables.
--
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
[GitHub] [iceberg] kbendick commented on pull request #5009: [WIP][CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#issuecomment-1151573825
After this change, here's an actual snapshot summary from Spark 3.2 for an unpartitioned table:
```json
{"spark.app.id":"local-1654805490169","added-data-files":"2","added-records":"3","added-files-size":"1286","changed-partition-count":"0","total-records":"3","total-files-size":"1286","total-data-files":"2","total-delete-files":"0","total-position-deletes":"0","total-equality-deletes":"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
[GitHub] [iceberg] kbendick commented on a diff in pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r919695184
##########
spark/v2.4/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java:
##########
@@ -783,7 +783,7 @@ public void testSnapshotsTable() {
.set("summary", ImmutableMap.of(
"added-records", "1",
"added-data-files", "1",
- "changed-partition-count", "1",
+ "changed-partition-count", "0",
Review Comment:
I agree that the root should be considered a partition of its own. Suppressing the root partition summary is sufficient.
Will update đź‘Ť
--
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
[GitHub] [iceberg] kbendick commented on pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#issuecomment-1155440960
This changes some of the existing behavior:
- `changed-partition-count` does not include "unpartitioned" - so the number of changed partitions could potentially go down by 1 in some scenarios (or not be present at all).
- We can decouple the function that writes the `partition.` stats and the count of changed partitions to keep the old behavior.
- Stops writing partition summaries for unpartitioned tables (the goal of the PR). So there won't be a `partitions.` summary in the file for the unpartitioned section of the table.
- Void partition transforms (dropped partitions) in v1 tables will still be included in partition summary stats as they transform to `alwaysNull` and where the change was made, it's not possible to differentiate between an actual partition field with a null value vs the void transform.
- This does not affect v2 tables which don't use the void transform
The big one is the `changed-partition-count`. We can decouple the logic so that `changed-partition-count` is still 1 for the unpartitioned parts of the table when they are touched (technically at present, if a V2 table has multiple unpartitioned specs, there will only be 1 value in total added to `changed-partition-count`, instead of the value being incremented for each unpartitioned section of the table).
If anybody has a real-world use case where the original behavior is needed, I'm very open to hearing it. My goal is just to remove `partition.` partition summary fields caused by the PartitionSpec for `PartitionSpec.unpartitioned()` being defined as an empty list of fields.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r919711098
##########
core/src/main/java/org/apache/iceberg/SnapshotSummary.java:
##########
@@ -186,7 +187,13 @@ public Map<String, String> build() {
metrics.addTo(builder);
setIf(deletedDuplicateFiles > 0, builder, DELETED_DUPLICATE_FILES, deletedDuplicateFiles);
- Set<String> changedPartitions = partitionMetrics.keySet();
+ // Exclude the key that corresponds to PartitionSpec.unpartitioned().
+ // We might want to keep it if and only if the table has had its partition spec updated over time and the
+ // write hit two specs (unpartitioned / VOID and an actual bound partition spec).
+ Set<String> changedPartitions = partitionMetrics.keySet()
+ .stream()
+ .filter(partKey -> partKey != null && !partKey.isEmpty())
+ .collect(Collectors.toSet());
Review Comment:
I considered that, but we don't have access to `isPartitioned` here. What we know about the table's partitioning comes from the metrics and the partition keys.
I agree that's cleaner but requires more of a refactor. I'm going to update to simply not write a summary for the root partition (i.e. unpartitioned tables).
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #5009: [WIP][CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r893967597
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -823,16 +823,14 @@ public void testAddDeleteFilesMultipleSpecs() {
Map<String, String> summary = snapshot.summary();
- Assert.assertEquals("Should change 4 partitions", "4", summary.get(CHANGED_PARTITION_COUNT_PROP));
+ Assert.assertEquals("Should change 3 partitions", "3", summary.get(CHANGED_PARTITION_COUNT_PROP));
Assert.assertEquals("Should add 1 data file", "1", summary.get(ADDED_FILES_PROP));
Assert.assertEquals("Should have 4 data files", "4", summary.get(TOTAL_DATA_FILES_PROP));
Assert.assertEquals("Should add 3 delete files", "3", summary.get(ADDED_DELETE_FILES_PROP));
Assert.assertEquals("Should have 3 delete files", "3", summary.get(TOTAL_DELETE_FILES_PROP));
Assert.assertEquals("Should add 3 position deletes", "3", summary.get(ADDED_POS_DELETES_PROP));
Assert.assertEquals("Should have 3 position deletes", "3", summary.get(TOTAL_POS_DELETES_PROP));
- Assert.assertTrue("Partition metrics must be correct",
- summary.get(CHANGED_PARTITION_PREFIX).contains(ADDED_DELETE_FILES_PROP + "=1"));
Review Comment:
We could also still increment `changed-partition-count` by 1, given that the `partitions` metadata table _does_ have a row for the "global partition" (i.e. there's a row count and files count that corresponds to the "unpartitioned" part of the 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] [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
URL: https://github.com/apache/iceberg/pull/5009
--
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 snapshot summary generating partition summary metrics for unpartitioned tables [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#issuecomment-2292492187
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r910195914
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -823,16 +823,14 @@ public void testAddDeleteFilesMultipleSpecs() {
Map<String, String> summary = snapshot.summary();
- Assert.assertEquals("Should change 4 partitions", "4", summary.get(CHANGED_PARTITION_COUNT_PROP));
+ Assert.assertEquals("Should change 3 partitions", "3", summary.get(CHANGED_PARTITION_COUNT_PROP));
Assert.assertEquals("Should add 1 data file", "1", summary.get(ADDED_FILES_PROP));
Assert.assertEquals("Should have 4 data files", "4", summary.get(TOTAL_DATA_FILES_PROP));
Assert.assertEquals("Should add 3 delete files", "3", summary.get(ADDED_DELETE_FILES_PROP));
Assert.assertEquals("Should have 3 delete files", "3", summary.get(TOTAL_DELETE_FILES_PROP));
Assert.assertEquals("Should add 3 position deletes", "3", summary.get(ADDED_POS_DELETES_PROP));
Assert.assertEquals("Should have 3 position deletes", "3", summary.get(TOTAL_POS_DELETES_PROP));
- Assert.assertTrue("Partition metrics must be correct",
- summary.get(CHANGED_PARTITION_PREFIX).contains(ADDED_DELETE_FILES_PROP + "=1"));
Review Comment:
I think this should still be present. It is okay to have an entry for the table root in cases like this.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #5009: [WIP][CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r893967597
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -823,16 +823,14 @@ public void testAddDeleteFilesMultipleSpecs() {
Map<String, String> summary = snapshot.summary();
- Assert.assertEquals("Should change 4 partitions", "4", summary.get(CHANGED_PARTITION_COUNT_PROP));
+ Assert.assertEquals("Should change 3 partitions", "3", summary.get(CHANGED_PARTITION_COUNT_PROP));
Assert.assertEquals("Should add 1 data file", "1", summary.get(ADDED_FILES_PROP));
Assert.assertEquals("Should have 4 data files", "4", summary.get(TOTAL_DATA_FILES_PROP));
Assert.assertEquals("Should add 3 delete files", "3", summary.get(ADDED_DELETE_FILES_PROP));
Assert.assertEquals("Should have 3 delete files", "3", summary.get(TOTAL_DELETE_FILES_PROP));
Assert.assertEquals("Should add 3 position deletes", "3", summary.get(ADDED_POS_DELETES_PROP));
Assert.assertEquals("Should have 3 position deletes", "3", summary.get(TOTAL_POS_DELETES_PROP));
- Assert.assertTrue("Partition metrics must be correct",
- summary.get(CHANGED_PARTITION_PREFIX).contains(ADDED_DELETE_FILES_PROP + "=1"));
Review Comment:
We could also still increment `changed-partition-count` by 1, given that the `partitions` metadata table _does_ have a row for the "global partition".
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r919712187
##########
core/src/main/java/org/apache/iceberg/SnapshotSummary.java:
##########
@@ -186,7 +187,13 @@ public Map<String, String> build() {
metrics.addTo(builder);
setIf(deletedDuplicateFiles > 0, builder, DELETED_DUPLICATE_FILES, deletedDuplicateFiles);
- Set<String> changedPartitions = partitionMetrics.keySet();
+ // Exclude the key that corresponds to PartitionSpec.unpartitioned().
+ // We might want to keep it if and only if the table has had its partition spec updated over time and the
+ // write hit two specs (unpartitioned / VOID and an actual bound partition spec).
+ Set<String> changedPartitions = partitionMetrics.keySet()
+ .stream()
+ .filter(partKey -> partKey != null && !partKey.isEmpty())
+ .collect(Collectors.toSet());
Review Comment:
And as mentioned elsewhere, we _do_ want to include some of the metrics about the root partition regardless (just not the 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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r910194993
##########
core/src/main/java/org/apache/iceberg/SnapshotSummary.java:
##########
@@ -186,7 +187,13 @@ public Map<String, String> build() {
metrics.addTo(builder);
setIf(deletedDuplicateFiles > 0, builder, DELETED_DUPLICATE_FILES, deletedDuplicateFiles);
- Set<String> changedPartitions = partitionMetrics.keySet();
+ // Exclude the key that corresponds to PartitionSpec.unpartitioned().
+ // We might want to keep it if and only if the table has had its partition spec updated over time and the
+ // write hit two specs (unpartitioned / VOID and an actual bound partition spec).
+ Set<String> changedPartitions = partitionMetrics.keySet()
+ .stream()
+ .filter(partKey -> partKey != null && !partKey.isEmpty())
+ .collect(Collectors.toSet());
Review Comment:
Why not just add a check on the line that adds partition summaries?
```java
if (trustPartitionMetrics && isPartitioned && changedPartitions.size() <= maxChangedPartitionsForSummaries) {
...
}
```
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #5009: [WIP][CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r893934029
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -823,16 +823,14 @@ public void testAddDeleteFilesMultipleSpecs() {
Map<String, String> summary = snapshot.summary();
- Assert.assertEquals("Should change 4 partitions", "4", summary.get(CHANGED_PARTITION_COUNT_PROP));
+ Assert.assertEquals("Should change 3 partitions", "3", summary.get(CHANGED_PARTITION_COUNT_PROP));
Assert.assertEquals("Should add 1 data file", "1", summary.get(ADDED_FILES_PROP));
Assert.assertEquals("Should have 4 data files", "4", summary.get(TOTAL_DATA_FILES_PROP));
Assert.assertEquals("Should add 3 delete files", "3", summary.get(ADDED_DELETE_FILES_PROP));
Assert.assertEquals("Should have 3 delete files", "3", summary.get(TOTAL_DELETE_FILES_PROP));
Assert.assertEquals("Should add 3 position deletes", "3", summary.get(ADDED_POS_DELETES_PROP));
Assert.assertEquals("Should have 3 position deletes", "3", summary.get(TOTAL_POS_DELETES_PROP));
- Assert.assertTrue("Partition metrics must be correct",
- summary.get(CHANGED_PARTITION_PREFIX).contains(ADDED_DELETE_FILES_PROP + "=1"));
Review Comment:
This partition summary field is for `"partition.": "..."` (the unpartitioned portion of the table).
As we're not writing summaries for unpartitioned fields, this needs to be removed.
We could in theory still add a "global partition" summary if partition summaries are enabled, but update it from `"partition."` to `"partition.unpartitioned"` or something.
--
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 snapshot summary generating partition summary metrics for unpartitioned tables [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#issuecomment-2309063950
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
[GitHub] [iceberg] kbendick commented on pull request #5009: [WIP][CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#issuecomment-1151566616
After this change, here's the actual summary (from the test) for both the unpartitioned table and the partitioned table. I also tested this with Spark (and have a unit test for it but felt it was not necessary for CI).
Unpartitioned
```json
{
"added-data-files": "1",
"added-records": "10",
"added-files-size": "350",
"changed-partition-count": "0",
"total-records": "10",
"total-files-size": "350",
"total-data-files": "1",
"total-delete-files": "0",
"total-position-deletes": "0",
"total-equality-deletes": "0"
}
```
Partitioned
```json
{
"added-data-files": "1",
"added-records": "10",
"added-files-size": "350",
"changed-partition-count": "1",
"partition-summaries-included": "true",
"partitions.data_bucket=0": "added-data-files=1,added-records=10,added-files-size=350",
"total-records": "10",
"total-files-size": "350",
"total-data-files": "1",
"total-delete-files": "0",
"total-position-deletes": "0",
"total-equality-deletes": "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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r910194993
##########
core/src/main/java/org/apache/iceberg/SnapshotSummary.java:
##########
@@ -186,7 +187,13 @@ public Map<String, String> build() {
metrics.addTo(builder);
setIf(deletedDuplicateFiles > 0, builder, DELETED_DUPLICATE_FILES, deletedDuplicateFiles);
- Set<String> changedPartitions = partitionMetrics.keySet();
+ // Exclude the key that corresponds to PartitionSpec.unpartitioned().
+ // We might want to keep it if and only if the table has had its partition spec updated over time and the
+ // write hit two specs (unpartitioned / VOID and an actual bound partition spec).
+ Set<String> changedPartitions = partitionMetrics.keySet()
+ .stream()
+ .filter(partKey -> partKey != null && !partKey.isEmpty())
+ .collect(Collectors.toSet());
Review Comment:
Why not just add a check on the line that adds partition summaries?
```java
if (trustPartitionMetrics && isPartitioned && changedPartitions.size() <= maxChangedPartitionsForSummaries) {
...
}
```
I think that's a cleaner way to do this.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5009: [CORE] - Fix snapshot summary generating partition summary metrics for unpartitioned tables
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5009:
URL: https://github.com/apache/iceberg/pull/5009#discussion_r910197381
##########
spark/v2.4/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java:
##########
@@ -783,7 +783,7 @@ public void testSnapshotsTable() {
.set("summary", ImmutableMap.of(
"added-records", "1",
"added-data-files", "1",
- "changed-partition-count", "1",
+ "changed-partition-count", "0",
Review Comment:
This depends on whether we consider the root to be a partition.
I think we should leave this as-is because there isn't a compelling reason to set this to 0 partitions, and that is a behavior change. We should suppress the root partition summary since it is identical to the table summary, but we shouldn't change this.
--
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