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/29 11:57:55 UTC
[GitHub] [iceberg] gaborkaszab opened a new pull request, #5159: Drop table's root folder if the table owns it
gaborkaszab opened a new pull request, #5159:
URL: https://github.com/apache/iceberg/pull/5159
Introducing a new Catalog level flag to indicate whether dropping a
table from HiveCatalog should also drop the whole location of the
table.
Testing:
- Created a new test that verifies that the location of the table
exists or not based on how the new flag is set.
- Manually loaded the changed jars into Apache Impala and checked
that setting the new flag will result the table location being
dropped.
--
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] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r928894935
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -112,6 +112,8 @@ public class TestHiveIcebergStorageHandlerNoScan {
private static final Set<String> IGNORED_PARAMS = ImmutableSet.of("bucketing_version", "numFilesErasureCoded");
+ private static final String DEFAULT_CATALOG_NAME = "default_iceberg";
+
Review Comment:
Not anymore. Dropped.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,17 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
+
+ Boolean shouldDropFullTableFolder = Boolean.parseBoolean(lastMetadata.property(
+ TableProperties.DROP_FULL_TABLE_FOLDER,
+ String.valueOf(TableProperties.DROP_FULL_TABLE_FOLDER_DEFAULT)));
+ if (shouldDropFullTableFolder) {
+ ((HadoopFileIO) fileIO).deletePrefix(lastMetadata.location());
Review Comment:
Indeed, thx!
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java:
##########
@@ -189,16 +189,37 @@ public Table createTable(TestHiveShell shell, String tableName, Schema schema, F
*/
public Table createTable(TestHiveShell shell, String tableName, Schema schema, PartitionSpec spec,
FileFormat fileFormat, List<Record> records) {
+ return createTable(shell, tableName, schema, spec, fileFormat, records, null);
+ }
+
+ /**
+ * Creates a partitioned Hive test table using Hive SQL. The table will be in the 'default' database.
+ * The table will be populated with the provided List of {@link Record}s using a Hive insert statement.
+ * @param shell The HiveShell used for Hive table creation
+ * @param tableName The name of the test table
+ * @param schema The schema used for the table creation
+ * @param spec The partition specification for the table
+ * @param fileFormat The file format used for writing the data
+ * @param records The records with which the table is populated
+ * @param properties Additional TblProperties to pass to the create table statement
+ * @return The created table
+ * @throws IOException If there is an error writing data
+ */
+ public Table createTable(TestHiveShell shell, String tableName, Schema schema, PartitionSpec spec,
Review Comment:
You mean I should put this test framework change into the Hive repo as well?
I can do so, I think the timing would be better after this PR is merged. Unless if there is any restriction that changes like this should land in Hive first.
--
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] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909693441
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -212,6 +216,46 @@ public void testCreateDropTable() throws TException, IOException, InterruptedExc
}
}
+ // This test exercises the catalog level 'drop-full-table-folder' setting to see if it has an effect on
+ // dropping the full table location or not.
Review Comment:
done
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,16 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
+
+ boolean tableOwnsLocation = PropertyUtil.propertyAsBoolean(catalogProperties,
+ CatalogProperties.DROP_FULL_TABLE_FOLDER, CatalogProperties.DROP_FULL_TABLE_FOLDER_DEFAULT);
+ if (tableOwnsLocation) {
+ fileIO.deleteFolder(lastMetadata.location());
+
+ LOG.info("Root directory deleted for table: {}", identifier);
Review Comment:
done
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909573453
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,16 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
+
+ boolean tableOwnsLocation = PropertyUtil.propertyAsBoolean(catalogProperties,
+ CatalogProperties.DROP_FULL_TABLE_FOLDER, CatalogProperties.DROP_FULL_TABLE_FOLDER_DEFAULT);
+ if (tableOwnsLocation) {
+ fileIO.deleteFolder(lastMetadata.location());
Review Comment:
OTOH with the setting `DROP_FULL_TABLE_FOLDER` we really assume that only this table puts files into these folders. If other table(s) put data to this folder the we would not like to use this config.
That said, we still might have problems with leftover uncommitted files which are not yet removed with `deleteOrphanFiles`
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909552556
##########
api/src/main/java/org/apache/iceberg/io/FileIO.java:
##########
@@ -61,6 +61,13 @@ default void deleteFile(OutputFile file) {
deleteFile(file.location());
}
+ /**
+ * Delete a folder at a given path.
+ */
+ default void deleteFolder(String path) {
+ deleteFile(path);
+ }
+
Review Comment:
We try to keep the FileIO as narrow as possible. Adding a new method could be problematic. How is this solved in other places where we try to remove directories?
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r928572649
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,17 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
Review Comment:
If we own the full table dir - can we expect that all of the files are in the given dir?
We might not need this `CatalogUtil.dropTableData(ops.io(), lastMetadata)` call when `shouldDropFullTableFolder` is `true`
--
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] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r928892440
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,17 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
Review Comment:
I think we can't expect that all of the files related to a table are in it's own directory. At least it's not enforced after setting the new tableproperty to keep all the files in metadata.location(). Additionally this tableproperty could be set and unset without checking the locations of the existing files.
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r928570571
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,17 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
+
+ Boolean shouldDropFullTableFolder = Boolean.parseBoolean(lastMetadata.property(
+ TableProperties.DROP_FULL_TABLE_FOLDER,
+ String.valueOf(TableProperties.DROP_FULL_TABLE_FOLDER_DEFAULT)));
+ if (shouldDropFullTableFolder) {
+ ((HadoopFileIO) fileIO).deletePrefix(lastMetadata.location());
Review Comment:
I this is not something we can build upon.
See:
```
this.fileIO = fileIOImpl == null ? new HadoopFileIO(conf) : CatalogUtil.loadFileIO(fileIOImpl, properties, conf);
```
I would do something like this:
```
if (fileIO instanceof SupportsPrefixOperations) {
((SupportsPrefixOperations) fileIO).deletePrefix(lastMetadata.location());
}
```
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909555684
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,16 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
+
+ boolean tableOwnsLocation = PropertyUtil.propertyAsBoolean(catalogProperties,
+ CatalogProperties.DROP_FULL_TABLE_FOLDER, CatalogProperties.DROP_FULL_TABLE_FOLDER_DEFAULT);
+ if (tableOwnsLocation) {
+ fileIO.deleteFolder(lastMetadata.location());
Review Comment:
Maybe we just remove the empty `data` and `metadata` folders, and then remove the table folder?
--
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] danielcweeks commented on pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#issuecomment-1204289036
The concept of location ownership and catalog behaviors around it are something that I feel we need to be more specific about. While this may work for specific uses of HiveCatalog, it very likely wouldn't be consistent across catalog implementations and even has some potential side-effects (e.g. renamed tables may end up sharing location and result in dropping data incorrectly).
These behaviors should really be owned by the Catalog (not the table) and should be tied to specific guarantees of the catalog implementation (i.e. de-conflicted/unique locations).
I agree we want to be able to support this behavior, but I don't feel like this approach is comprehensive enough to address the problem.
--
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] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r924423375
##########
api/src/main/java/org/apache/iceberg/io/FileIO.java:
##########
@@ -61,6 +61,13 @@ default void deleteFile(OutputFile file) {
deleteFile(file.location());
}
+ /**
+ * Delete a folder at a given path.
+ */
+ default void deleteFolder(String path) {
+ deleteFile(path);
+ }
+
Review Comment:
Aaah, this code is relatively new, I didn't have it in my local copy. HadoopFileIO.deletePrefix() is actually the same I implemented now, so apparently someone also needed this but was a bit faster than me :)
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909557470
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -212,6 +216,46 @@ public void testCreateDropTable() throws TException, IOException, InterruptedExc
}
}
+ // This test exercises the catalog level 'drop-full-table-folder' setting to see if it has an effect on
+ // dropping the full table location or not.
+ @Test
+ public void testDropTableFolderOwnershipForHiveCatalog()
Review Comment:
Maybe another test case when we do not drop the dirs?
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909550727
##########
api/src/main/java/org/apache/iceberg/io/FileIO.java:
##########
@@ -61,6 +61,13 @@ default void deleteFile(OutputFile file) {
deleteFile(file.location());
}
+ /**
+ * Delete a folder at a given path.
+ */
+ default void deleteFolder(String path) {
+ deleteFile(path);
+ }
+
Review Comment:
Do we need 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] gaborkaszab commented on pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#issuecomment-1170072586
@rdblue well my idea was to keep this as simple as possible and by ownership here I meant that the table should drop whatever is in TableMetadata.location().
The reason I think this could work is by setting a Catalog level flag to indicate that tables will drop everything in their root dir, we actually also say that make sure that one table only populates the directory under TableMetadata.location() related to the table and won't spread their files into other locations.
--
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] gaborkaszab commented on pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#issuecomment-1169997303
@pvary I uploaded a new patch where I got rid of the FileIO changes. In turn I had to check the type for 'fileIO' member of HiveCatalog so that I can call deleteRecusrive()
If you are not a fan of this approach we can simply call deleteFile() for data/, metadata/ and the table's location but this seems error-prone for me as we can't really guarantee that those 2 folders will be the only content of a table's directory. A recursive delete seems simpler and error-proof.
--
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] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909565489
##########
api/src/main/java/org/apache/iceberg/io/FileIO.java:
##########
@@ -61,6 +61,13 @@ default void deleteFile(OutputFile file) {
deleteFile(file.location());
}
+ /**
+ * Delete a folder at a given path.
+ */
+ default void deleteFolder(String path) {
+ deleteFile(path);
+ }
+
Review Comment:
Currently only empty folders could be dropped as the 'isRecursive' flag is hardcoded to false in HadoopFileIO.
If we want to keep FileIO intact, we can then change new logic in HiveCatalog.dropTable() to check if the FileIO is of HadoopFileIO and use its interface to invoke dropFolder() (or could be named dropRecursive()) instead of through FileIO.
--
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] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909566933
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopFileIO.java:
##########
@@ -66,10 +66,19 @@ public OutputFile newOutputFile(String path) {
@Override
public void deleteFile(String path) {
+ deleteImpl(path, false);
+ }
+
+ @Override
+ public void deleteFolder(String path) {
+ deleteImpl(path, true);
+ }
+
+ private void deleteImpl(String path, boolean recursive) {
Path toDelete = new Path(path);
FileSystem fs = Util.getFs(toDelete, hadoopConf.get());
try {
- fs.delete(toDelete, false /* not recursive */);
+ fs.delete(toDelete, recursive);
Review Comment:
For that we would need to list the files in the directory so that we can drop them one-by-one. I'd go for simple invoke a recursive drop here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909579239
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,16 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
+
+ boolean tableOwnsLocation = PropertyUtil.propertyAsBoolean(catalogProperties,
+ CatalogProperties.DROP_FULL_TABLE_FOLDER, CatalogProperties.DROP_FULL_TABLE_FOLDER_DEFAULT);
+ if (tableOwnsLocation) {
+ fileIO.deleteFolder(lastMetadata.location());
Review Comment:
That's true. There is one thing I wasn't sure about. I saw some old issues (also related to dropTable) where the root table dir not only contained a data/ and metadata/ folder but also the different partitions had some own directories under data/.
I'm not sure this could still happen, and I didn't manage to repro such a case but if in fact there are tables with such a directory structure the proposed approach wouldn't be able to drop the root dir then.
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909557064
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -212,6 +216,46 @@ public void testCreateDropTable() throws TException, IOException, InterruptedExc
}
}
+ // This test exercises the catalog level 'drop-full-table-folder' setting to see if it has an effect on
+ // dropping the full table location or not.
Review Comment:
Maybe a proper javadoc?
--
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 pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#issuecomment-1170026252
I think we need to formalize the idea of ownership. What paths does a table own and how do we know that? I think we should probably have something in the table metadata to track 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] gaborkaszab commented on pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#issuecomment-1190352837
@rdblue I'm not sure I got your preference from you comment but I changed the code to have a table-level property instead of a catalog-level flag to control this functionality. I hope this is what you had in mind.
--
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] pvary commented on pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#issuecomment-1193718724
@gaborkaszab I think the main question is if we set the table property for owning the full directory then do we expect that all of the files are in the given directory, and there is no files outside of this directory. If we accept/expect this then we can simplify the delete process
--
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] gaborkaszab commented on pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#issuecomment-1198115902
@rdblue thanks for the review!
I changed the patch to have a tableproperty that is a list of locations that the table owns, and that are dropped when the table is dropped. I'm looking forward to hear you opinion!
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909555101
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopFileIO.java:
##########
@@ -66,10 +66,19 @@ public OutputFile newOutputFile(String path) {
@Override
public void deleteFile(String path) {
+ deleteImpl(path, false);
+ }
+
+ @Override
+ public void deleteFolder(String path) {
+ deleteImpl(path, true);
+ }
+
+ private void deleteImpl(String path, boolean recursive) {
Path toDelete = new Path(path);
FileSystem fs = Util.getFs(toDelete, hadoopConf.get());
try {
- fs.delete(toDelete, false /* not recursive */);
+ fs.delete(toDelete, recursive);
Review Comment:
Is the directory empty here?
If not, could we first remove the files in it?
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909557746
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -212,6 +216,46 @@ public void testCreateDropTable() throws TException, IOException, InterruptedExc
}
}
+ // This test exercises the catalog level 'drop-full-table-folder' setting to see if it has an effect on
+ // dropping the full table location or not.
+ @Test
+ public void testDropTableFolderOwnershipForHiveCatalog()
Review Comment:
If it is not tested somewhere else?
--
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] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909569578
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -212,6 +216,46 @@ public void testCreateDropTable() throws TException, IOException, InterruptedExc
}
}
+ // This test exercises the catalog level 'drop-full-table-folder' setting to see if it has an effect on
+ // dropping the full table location or not.
+ @Test
+ public void testDropTableFolderOwnershipForHiveCatalog()
Review Comment:
This test also covers when the setting is off and we don't drop the table dir.
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909685183
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,16 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
+
+ boolean tableOwnsLocation = PropertyUtil.propertyAsBoolean(catalogProperties,
+ CatalogProperties.DROP_FULL_TABLE_FOLDER, CatalogProperties.DROP_FULL_TABLE_FOLDER_DEFAULT);
+ if (tableOwnsLocation) {
+ fileIO.deleteFolder(lastMetadata.location());
Review Comment:
For the record: we should check partitioned tables as well. I expect that the partition directories will remain as well
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r928576574
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java:
##########
@@ -189,16 +189,37 @@ public Table createTable(TestHiveShell shell, String tableName, Schema schema, F
*/
public Table createTable(TestHiveShell shell, String tableName, Schema schema, PartitionSpec spec,
FileFormat fileFormat, List<Record> records) {
+ return createTable(shell, tableName, schema, spec, fileFormat, records, null);
+ }
+
+ /**
+ * Creates a partitioned Hive test table using Hive SQL. The table will be in the 'default' database.
+ * The table will be populated with the provided List of {@link Record}s using a Hive insert statement.
+ * @param shell The HiveShell used for Hive table creation
+ * @param tableName The name of the test table
+ * @param schema The schema used for the table creation
+ * @param spec The partition specification for the table
+ * @param fileFormat The file format used for writing the data
+ * @param records The records with which the table is populated
+ * @param properties Additional TblProperties to pass to the create table statement
+ * @return The created table
+ * @throws IOException If there is an error writing data
+ */
+ public Table createTable(TestHiveShell shell, String tableName, Schema schema, PartitionSpec spec,
Review Comment:
Could we base this change the already existing hive code?
https://github.com/apache/hive/blob/90428cc5f594bd0abb457e4e5c391007b2ad1cb8/iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/TestTables.java#L338-L356
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r909554014
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -171,9 +172,16 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
false /* throw NoSuchObjectException if the table doesn't exist */);
return null;
});
-
if (purge && lastMetadata != null) {
CatalogUtil.dropTableData(ops.io(), lastMetadata);
+
+ boolean tableOwnsLocation = PropertyUtil.propertyAsBoolean(catalogProperties,
+ CatalogProperties.DROP_FULL_TABLE_FOLDER, CatalogProperties.DROP_FULL_TABLE_FOLDER_DEFAULT);
+ if (tableOwnsLocation) {
+ fileIO.deleteFolder(lastMetadata.location());
+
+ LOG.info("Root directory deleted for table: {}", identifier);
Review Comment:
Maybe debug level log should be enough
--
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] gaborkaszab commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r953684886
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -177,20 +179,17 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
}
try {
- clients.run(
- client -> {
- client.dropTable(
- database,
- identifier.name(),
- false /* do not delete data */,
- false /* throw NoSuchObjectException if the table doesn't exist */);
- return null;
- });
-
+ clients.run(client -> {
+ client.dropTable(database, identifier.name(),
+ false /* do not delete data */,
+ false /* throw NoSuchObjectException if the table doesn't exist */);
+ return null;
+ });
Review Comment:
Thanks @wypoon for your comment!
Yes, I meanwhile I also learnt that this PR was a bit naive and it involves a much higher level topic about Catalogs being responsible to guarantee that dropping table location is safe.
I think I'll close this PR, then.
--
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] gaborkaszab closed pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
gaborkaszab closed pull request #5159: Drop table's root folder if the table owns it
URL: https://github.com/apache/iceberg/pull/5159
--
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] wypoon commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
wypoon commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r953303333
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -177,20 +179,17 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
}
try {
- clients.run(
- client -> {
- client.dropTable(
- database,
- identifier.name(),
- false /* do not delete data */,
- false /* throw NoSuchObjectException if the table doesn't exist */);
- return null;
- });
-
+ clients.run(client -> {
+ client.dropTable(database, identifier.name(),
+ false /* do not delete data */,
+ false /* throw NoSuchObjectException if the table doesn't exist */);
+ return null;
+ });
Review Comment:
This seems to be a purely formatting change. It is unnecessary.
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -345,4 +345,11 @@ private TableProperties() {}
public static final String UPSERT_ENABLED = "write.upsert.enabled";
public static final boolean UPSERT_ENABLED_DEFAULT = false;
+
+ /**
+ * Table properties to be used when dropping a table. Contains a list of locations that are owned
+ * by the table and should also be dropped.
+ */
+ public static final String LOCATIONS_OWNED_BY_TABLE = "locations-owned-by-table";
Review Comment:
How would this property be populated? Does the user have to specify this explicitly per table? Or will a catalog auto-populate it?
--
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 pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#issuecomment-1194508860
@gaborkaszab, by adding a table property you're defining ownership. The problem is, you're adding an ownership concept that we know is going to change (we will need multiple roots) and doing it with very specific support (as @pvary mentioned, the assumption is that the directory is owned AND that no files are outside of it). As I said, I think the solution is to add a real concept of location ownership rather than adding temporary work-arounds.
What about updating the spec to have a list of owned locations and using that to fix 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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r928570883
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -112,6 +112,8 @@ public class TestHiveIcebergStorageHandlerNoScan {
private static final Set<String> IGNORED_PARAMS = ImmutableSet.of("bucketing_version", "numFilesErasureCoded");
+ private static final String DEFAULT_CATALOG_NAME = "default_iceberg";
+
Review Comment:
Is this needed?
--
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] pvary commented on a diff in pull request #5159: Drop table's root folder if the table owns it
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5159:
URL: https://github.com/apache/iceberg/pull/5159#discussion_r911081629
##########
api/src/main/java/org/apache/iceberg/io/FileIO.java:
##########
@@ -61,6 +61,13 @@ default void deleteFile(OutputFile file) {
deleteFile(file.location());
}
+ /**
+ * Delete a folder at a given path.
+ */
+ default void deleteFolder(String path) {
+ deleteFile(path);
+ }
+
Review Comment:
Maybe `SupportsPrefixOperations.deletePrefix` could help us here.
See the `HadoopFileIO.deletePrefix` implementation.
--
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