You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "hsiang-c (via GitHub)" <gi...@apache.org> on 2025/03/13 02:32:48 UTC
[PR] Core: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
hsiang-c opened a new pull request, #12511:
URL: https://github.com/apache/iceberg/pull/12511
- `HiveCatalog` implemented `dropView` in https://github.com/apache/iceberg/pull/9852 and the view metadata will be purged from storage if `TableProperties.GC_ENABLED` is `true`(default)
- I think `JdbcCatalog` should also purge view metadata in the same way.
- Tests are borrowed from https://github.com/apache/iceberg/pull/9852/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
Re: [PR] Core: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1996505373
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
+ ViewMetadata lastViewMetadata = null;
+ try {
+ lastViewMetadata = ops.current();
+ } catch (NotFoundException e) {
Review Comment:
`ops.current()` throws `NoSuchViewException` and `NotFoundException`.
When `NoSuchViewException` is thrown, it means the view to drop doesn't exist. I wonder if it still makes sense to continue executing the following `JdbcUtil.DROP_VIEW_SQL`.
When `NotFoundException` is thrown by the `refreshFromMetadataLocation(newMetadataLocation)` call in `JdbcViewOperations#doRefresh()`, it means the view exists but `newMetadataLocation` is not found while we try to read the metadata by `ViewMetadataParser.read()`. I think executing `JdbcUtil.DROP_VIEW_SQL` makes sense.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1994659921
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
Review Comment:
Why not add these tests to `ViewCatalogTests`?
Can we add `test` prefix for consistency with other test methods? Also, `throws IOException` is redundant.
Same for another test.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r2002321359
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
Review Comment:
I think I'd prefer to leave it as is at least for now. IMO the issue with upleveling `FileIO io()` to ViewOperations is that implementations like RESTViewOperations don't really need a FileIO instance to be able to write out or read metadata (in the REST case it's all over the protocol). So the concept doesn't really apply to every instance of `ViewOperations`.
Maybe there's a mixin or something to make this cleaner, but I wouldn't introduce it 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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1996443096
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
+ ViewMetadata lastViewMetadata = null;
+ try {
+ lastViewMetadata = ops.current();
+ } catch (NotFoundException e) {
Review Comment:
Good catch, thanks.
`BaseViewOperations`'s `refresh()` method throws `NoSuchViewException`
I also applied the fix to `dropTable` b/c `BaseMetastoreTableOperations`'s `refresh()` method throws `NoSuchTableException`.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1994659921
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
Review Comment:
Why not add these tests to `ViewCatalogTests`? Also, `throws IOException` is redundant.
Same for another test.
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
Review Comment:
Why not add these tests to `ViewCatalogTests`? Also, `throws IOException` is redundant.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1996433987
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
Review Comment:
@ebyhr I gave it a try and I think the file existence check doesn't work for `InMemoryCatalog` and therefore `TestInMemoryViewCatalog` fails.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1993182575
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -18,14 +18,18 @@
*/
package org.apache.iceberg.jdbc;
+import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
+import java.io.IOException;
import java.sql.SQLException;
import java.util.Map;
import java.util.UUID;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
Review Comment:
this shouldn't be a Hadoop Path
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#issuecomment-2735196052
Thank you @hsiang-c for this, and thanks @ebyhr @nastra for reviewing!
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1996433987
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
Review Comment:
@ebyhr
I gave it a try and I think the file existence check doesn't work for `InMemoryCatalog` and therefore `TestInMemoryViewCatalog` fails. `
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
Review Comment:
@ebyhr
I gave it a try and I think the file existence check doesn't work for `InMemoryCatalog` and therefore `TestInMemoryViewCatalog` fails.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1996443096
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
+ ViewMetadata lastViewMetadata = null;
+ try {
+ lastViewMetadata = ops.current();
+ } catch (NotFoundException e) {
Review Comment:
Good catch, thanks.
`BaseViewOperations`'s `refresh()` method throws `NoSuchViewException`
I also applied the fix to `dropTable` b/c `BaseMetastoreTableOperations`'s `refresh()` method throws `NoSuchTableException`.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #12511:
URL: https://github.com/apache/iceberg/pull/12511
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#issuecomment-2719907863
cc @jbonofre @ajantha-bhat Please take a look for me, thanks.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1993961102
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -18,14 +18,18 @@
*/
package org.apache.iceberg.jdbc;
+import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
+import java.io.IOException;
import java.sql.SQLException;
import java.util.Map;
import java.util.UUID;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
Review Comment:
Thanks @nastra, I replace it with `java.io.File`.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r2002508260
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
Review Comment:
ah sorry I missed that you're using `io()` further below. I would also leave things as they are and not move `io()` to View/TableOperations
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r2000321092
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
Review Comment:
```suggestion
ViewOperations ops = newViewOps(identifier);
```
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r2003819845
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
Review Comment:
@amogh-jahagirdar @nastra Thank you for your feedback.
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1996039962
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
Review Comment:
>Why not add these tests to ViewCatalogTests
Good idea
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r1994676612
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
+ ViewMetadata lastViewMetadata = null;
+ try {
+ lastViewMetadata = ops.current();
+ } catch (NotFoundException e) {
Review Comment:
When does `ops.current()` method throw `NotFoundException`? I think it throws `NoSuchViewException`.
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
+ TableIdentifier identifier = TableIdentifier.of("namespace1", "view");
+ BaseView view =
+ (BaseView)
+ catalog
+ .buildView(identifier)
+ .withQuery("spark", "select * from tbl")
+ .withSchema(SCHEMA)
+ .withDefaultNamespace(Namespace.of("namespace1"))
+ .withProperty(TableProperties.GC_ENABLED, "false")
+ .create();
+
+ assertThat(catalog.viewExists(identifier)).isTrue();
+ String metadataFileLocation = view.operations().current().metadataFileLocation();
+ assertThat(metadataFileLocation).isNotNull();
+ File currentMetadataLocation = new File(metadataFileLocation);
+
+ catalog.dropView(identifier);
+ assertThat(currentMetadataLocation.exists()).isTrue();
Review Comment:
AssertJ has a method to verify a file existence:
```suggestion
assertThat(currentMetadataLocation).exists();
```
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
Review Comment:
Why do we want to test only JDBC catalog?
Can we add `test` prefix for consistency with other test methods? Also, `throws IOException` is redundant.
Same for another test.
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcViewCatalog.java:
##########
@@ -138,4 +142,50 @@ public void testCommitExceptionWithMessage() {
.hasMessageStartingWith("View already exists: " + identifier);
}
}
+
+ @Test
+ public void dropViewShouldNotDropMetadataFileIfGcNotEnabled() throws IOException {
+ TableIdentifier identifier = TableIdentifier.of("namespace1", "view");
+ BaseView view =
+ (BaseView)
+ catalog
+ .buildView(identifier)
+ .withQuery("spark", "select * from tbl")
+ .withSchema(SCHEMA)
+ .withDefaultNamespace(Namespace.of("namespace1"))
+ .withProperty(TableProperties.GC_ENABLED, "false")
+ .create();
+
+ assertThat(catalog.viewExists(identifier)).isTrue();
+ String metadataFileLocation = view.operations().current().metadataFileLocation();
+ assertThat(metadataFileLocation).isNotNull();
+ File currentMetadataLocation = new File(metadataFileLocation);
+
+ catalog.dropView(identifier);
+ assertThat(currentMetadataLocation.exists()).isTrue();
+ assertThat(catalog.viewExists(identifier)).isFalse();
+ }
+
+ @Test
+ public void dropViewShouldDropMetadataFileIfGcEnabled() throws IOException {
+ TableIdentifier identifier = TableIdentifier.of("namespace1", "view");
+ BaseView view =
+ (BaseView)
+ catalog
+ .buildView(identifier)
+ .withQuery("spark", "select * from tbl")
+ .withSchema(SCHEMA)
+ .withDefaultNamespace(Namespace.of("namespace1"))
+ .withProperty(TableProperties.GC_ENABLED, "true")
+ .create();
+
+ assertThat(catalog.viewExists(identifier)).isTrue();
+ String metadataFileLocation = view.operations().current().metadataFileLocation();
+ assertThat(metadataFileLocation).isNotNull();
+ File currentMetadataLocation = new File(metadataFileLocation);
+
+ catalog.dropView(identifier);
+ assertThat(currentMetadataLocation.exists()).isFalse();
Review Comment:
```suggestion
assertThat(currentMetadataLocation).doesNotExist();
```
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r2001677003
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
Review Comment:
@nastra
The `io()` method is defined in `BaseViewOperations` instead of `ViewOperations` and I need to call `ops.io()` to drop view's metadata.
Perhaps we could define `FileIO io()` in the `ViewOperations` interface, just like `TableOperations`?
--
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: JDBCCatalog's dropView() should purge metadata files if GC is enabled [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #12511:
URL: https://github.com/apache/iceberg/pull/12511#discussion_r2002321359
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -612,6 +613,14 @@ public boolean dropView(TableIdentifier identifier) {
throw new UnsupportedOperationException(VIEW_WARNING_LOG_MESSAGE);
}
+ JdbcViewOperations ops = (JdbcViewOperations) newViewOps(identifier);
Review Comment:
I think I'd prefer to leave it as is at least for now. IMO the issue with upleveling `FileIO io()` to ViewOperations is that implementations like RESTViewOperations don't really need a FileIO instance to be able to write out or commit metadata (in the REST case it's all over the protocol). So the concept doesn't really apply to every instance of `ViewOperations`.
Maybe there's a mixin or something to make this cleaner, but I wouldn't introduce it 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