You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2025/04/16 09:39:39 UTC
[PR] Core: Test loading table/view with non-existing namespace [iceberg]
nastra opened a new pull request, #12812:
URL: https://github.com/apache/iceberg/pull/12812
(no comment)
--
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: Test loading table/view with non-existing namespace [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #12812:
URL: https://github.com/apache/iceberg/pull/12812#discussion_r2046630495
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -917,6 +917,15 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void loadTableWithNonExistingNamespace() {
Review Comment:
To be consistent, better name it `testLoadTableWithNonExistingNamespace`
--
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: Test loading table/view with non-existing namespace [iceberg]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on code in PR #12812:
URL: https://github.com/apache/iceberg/pull/12812#discussion_r2049292649
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -917,6 +917,15 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void testLoadTableWithNonExistingNamespace() {
+ TableIdentifier ident = TableIdentifier.of("non-existing", "tbl");
+ assertThat(catalog().tableExists(ident)).as("Table should not exist").isFalse();
+ assertThatThrownBy(() -> catalog().loadTable(ident))
+ .isInstanceOf(NoSuchTableException.class)
+ .hasMessageStartingWith("Table does not exist: %s", ident);
Review Comment:
I wonder if we can make more precise assertion than `.hasMessageStartingWith` just to avoid (unlikely) prefix match.
It seems `hasMessageEndingWith` is a better choice.
--
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: Test loading table/view with non-existing namespace [iceberg]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on code in PR #12812:
URL: https://github.com/apache/iceberg/pull/12812#discussion_r2049360573
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -917,6 +917,15 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void testLoadTableWithNonExistingNamespace() {
+ TableIdentifier ident = TableIdentifier.of("non-existing", "tbl");
+ assertThat(catalog().tableExists(ident)).as("Table should not exist").isFalse();
+ assertThatThrownBy(() -> catalog().loadTable(ident))
+ .isInstanceOf(NoSuchTableException.class)
+ .hasMessageStartingWith("Table does not exist: %s", ident);
Review Comment:
```
throw new NoSuchTableException("Table does not exist: %s", identifier);
```
I wonder if punctuation (or a space) should be added at the end of message where the exception is thrown so that we eliminate the chance of false positive.
--
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: Test loading table/view with non-existing namespace [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12812:
URL: https://github.com/apache/iceberg/pull/12812#discussion_r2049340044
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -917,6 +917,15 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void testLoadTableWithNonExistingNamespace() {
+ TableIdentifier ident = TableIdentifier.of("non-existing", "tbl");
+ assertThat(catalog().tableExists(ident)).as("Table should not exist").isFalse();
+ assertThatThrownBy(() -> catalog().loadTable(ident))
+ .isInstanceOf(NoSuchTableException.class)
+ .hasMessageStartingWith("Table does not exist: %s", ident);
Review Comment:
there are catalog implementations that have additional details at the end of the message, hence we're checking the beginning of the error msg
--
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: Test loading table/view with non-existing namespace [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #12812:
URL: https://github.com/apache/iceberg/pull/12812
--
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: Test loading table/view with non-existing namespace [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12812:
URL: https://github.com/apache/iceberg/pull/12812#discussion_r2046687362
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -917,6 +917,15 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void loadTableWithNonExistingNamespace() {
Review Comment:
```suggestion
public void testLoadTableWithNonExistingNamespace() {
```
--
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: Test loading table/view with non-existing namespace [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #12812:
URL: https://github.com/apache/iceberg/pull/12812#issuecomment-2809235118
thanks for the reviews @manuzhang @pvary @Fokko
--
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: Test loading table/view with non-existing namespace [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12812:
URL: https://github.com/apache/iceberg/pull/12812#discussion_r2046690017
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -917,6 +917,15 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void loadTableWithNonExistingNamespace() {
Review Comment:
good point, updated
--
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