You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dramaticlly (via GitHub)" <gi...@apache.org> on 2023/12/14 19:48:55 UTC
[PR] Core: remove statistic files in CatalogUtil:dropTableData [iceberg]
dramaticlly opened a new pull request, #9305:
URL: https://github.com/apache/iceberg/pull/9305
This PR attempt to include puffin stats files as part of CatalogUtil table cleanup
CC @RussellSpitzer @findepi
--
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: remove statistic files in CatalogUtil:dropTableData [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #9305:
URL: https://github.com/apache/iceberg/pull/9305#discussion_r1427226753
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -100,7 +100,7 @@ public static void dropTableData(FileIO io, TableMetadata metadata) {
LOG.info("Manifests to delete: {}", Joiner.on(", ").join(manifestsToDelete));
- // run all of the deletes
+ // run all the deletes
Review Comment:
IntelliJ trying to be smart at syntax so I let it fix but I can revert!
--
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: remove statistic files in CatalogUtil:dropTableData [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #9305:
URL: https://github.com/apache/iceberg/pull/9305#issuecomment-1876393081
> There is also a partition stats file added recently https://github.com/apache/iceberg/commit/6e21bbf4c4cd2c8351a64636f91d05d00492dff2
We should handle this for them aswell.
I will open the follow up PR for partition stats files today.
--
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: remove statistic files in CatalogUtil:dropTableData [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #9305:
URL: https://github.com/apache/iceberg/pull/9305#discussion_r1430486073
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -117,6 +117,11 @@ public static void dropTableData(FileIO io, TableMetadata metadata) {
Iterables.transform(metadata.previousFiles(), TableMetadata.MetadataLogEntry::file),
"previous metadata",
true);
+ deleteFiles(
+ io,
+ Iterables.transform(metadata.statisticsFiles(), StatisticsFile::path),
+ "statistic",
Review Comment:
thank you @ajantha-bhat , I used `statistic` earlier due to singular nouns here given how all existing code refer to file type, like `manifest`, `manifest list` and `previous metadata`. But I now recognize that the class name is already defined as `api/src/main/java/org/apache/iceberg/StatisticsFile.java`. let me update to your suggestion
--
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: remove statistic files in CatalogUtil:dropTableData [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #9305:
URL: https://github.com/apache/iceberg/pull/9305
--
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: remove statistic files in CatalogUtil:dropTableData [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #9305:
URL: https://github.com/apache/iceberg/pull/9305#discussion_r1427225369
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -100,7 +100,7 @@ public static void dropTableData(FileIO io, TableMetadata metadata) {
LOG.info("Manifests to delete: {}", Joiner.on(", ").join(manifestsToDelete));
- // run all of the deletes
+ // run all the deletes
Review Comment:
Probably fine as is?
--
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: remove statistic files in CatalogUtil:dropTableData [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #9305:
URL: https://github.com/apache/iceberg/pull/9305#issuecomment-1864264195
There is also a partition stats file added recently https://github.com/apache/iceberg/commit/6e21bbf4c4cd2c8351a64636f91d05d00492dff2
We should handle this for them aswell.
I can have a follow up PR, if it is too much to handle in this 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] Core: remove statistic files in CatalogUtil:dropTableData [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #9305:
URL: https://github.com/apache/iceberg/pull/9305#discussion_r1432657742
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -181,4 +205,34 @@ private Set<String> metadataLocations(TableMetadata tableMetadata) {
metadataLocations.add(tableMetadata.metadataFileLocation());
return metadataLocations;
}
+
+ private Set<String> statsLocations(TableMetadata tableMetadata) {
Review Comment:
nit: these two new private methods can be static
--
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: remove statistic files in CatalogUtil:dropTableData [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #9305:
URL: https://github.com/apache/iceberg/pull/9305#discussion_r1427885903
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -38,19 +49,28 @@
public class TestCatalogUtilDropTable extends HadoopTableTestBase {
@Test
- public void dropTableDataDeletesExpectedFiles() {
+ public void dropTableDataDeletesExpectedFiles() throws IOException {
table.newFastAppend().appendFile(FILE_A).commit();
table.newAppend().appendFile(FILE_B).commit();
+ StatisticsFile statisticsFile =
+ writeStatsFile(
+ table.currentSnapshot().snapshotId(),
+ table.currentSnapshot().sequenceNumber(),
+ tableLocation + "/metadata/" + UUID.randomUUID() + ".stats",
+ table.io());
+ table.updateStatistics().setStatistics(statisticsFile.snapshotId(), statisticsFile).commit();
- TableMetadata tableMetadata = readMetadataVersion(3);
+ TableMetadata tableMetadata = readMetadataVersion(4);
Set<Snapshot> snapshotSet = Sets.newHashSet(table.snapshots());
Set<String> manifestListLocations = manifestListLocations(snapshotSet);
Set<String> manifestLocations = manifestLocations(snapshotSet, table.io());
Set<String> dataLocations = dataLocations(snapshotSet, table.io());
Set<String> metadataLocations = metadataLocations(tableMetadata);
+ Set<String> statisticLocations = statisticLocations(tableMetadata);
Review Comment:
this variable name and method name can be statisticsLocations ('s' is missing) to be inline with naming of other places or just statsLocations is also fine.
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -117,6 +117,11 @@ public static void dropTableData(FileIO io, TableMetadata metadata) {
Iterables.transform(metadata.previousFiles(), TableMetadata.MetadataLogEntry::file),
"previous metadata",
true);
+ deleteFiles(
+ io,
+ Iterables.transform(metadata.statisticsFiles(), StatisticsFile::path),
+ "statistic",
Review Comment:
```suggestion
"statistics",
```
--
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