You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "talatuyarer (via GitHub)" <gi...@apache.org> on 2025/04/11 00:46:43 UTC
[PR] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
talatuyarer opened a new pull request, #12768:
URL: https://github.com/apache/iceberg/pull/12768
Hi all,
This PR addresses several improvements to the `CatalogTests` class:
1. **`listNamespacesWithEmptyNamespace` Fix:** It corrects a bug where the `listNamespacesWithEmptyNamespace` test doesn't properly check for `supportsEmptyNamespace` on catalogs. This ensures the test is only executed for catalogs that support empty namespaces, preventing potential errors or misleading test results.
2. **Reserved Keyword Avoidance:** The tests were using `TABLE` as a table identifier, which is a reserved keyword in ANSI SQL and various database systems (e.g., [Spark](https://docs.databricks.com/aws/en/sql/language-manual/sql-ref-reserved-words)). This PR renames the identifier to `newtable` to avoid conflicts and improve compatibility with a wider range of catalogs.
3. **Configurable Table Location:** Currently, the `CatalogTests` class hardcodes table locations, preventing catalog implementations from easily overriding this behavior for their specific needs. This PR introduces a mechanism to configure the base table location via the `testTableBaseLocation() ` method, with a default value of `file:/tmp`. This allows sub-classes to adapt the tests to different storage systems and environments.
These changes enhance the robustness, compatibility, and flexibility of the `CatalogTests` suite, making it more effective for verifying catalog implementations.
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2041558209
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2636,7 +2636,11 @@ public void testTableExistsViaHEADRequest() {
any());
Mockito.verify(adapter)
.execute(
- reqMatcher(HTTPMethod.HEAD, "v1/namespaces/newdb/tables/table", Map.of(), Map.of()),
+ reqMatcher(
+ HTTPMethod.HEAD,
+ "v1/namespaces/" + TABLE.namespace() + "/tables/" + TABLE.name(),
Review Comment:
you should be able to replace this with `ResourcePaths.forCatalogProperties(Map.of()).table(TABLE)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2045212696
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1256,37 +1255,37 @@ public void testTableAuth(
required(2, "data", Types.StringType.get()));
if (requiresNamespaceCreate()) {
- catalog.createNamespace(ident.namespace());
+ catalog.createNamespace(TBL.namespace());
}
- Table table = catalog.createTable(ident, expectedSchema);
+ Table table = catalog.createTable(TBL, expectedSchema);
assertThat(table.schema().asStruct())
.as("Schema should match")
.isEqualTo(expectedSchema.asStruct());
- Table loaded = catalog.loadTable(ident); // the first load will send the token
+ Table loaded = catalog.loadTable(TBL); // the first load will send the token
assertThat(loaded.schema().asStruct())
.as("Schema should match")
.isEqualTo(expectedSchema.asStruct());
loaded.refresh(); // refresh to force reload
- Mockito.verify(adapter)
+ verify(adapter)
Review Comment:
Looks like Intellij Idea did a refactoring without telling me :D I reverted those changes and apply only ident changes.
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2041562055
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2636,7 +2636,11 @@ public void testTableExistsViaHEADRequest() {
any());
Mockito.verify(adapter)
.execute(
- reqMatcher(HTTPMethod.HEAD, "v1/namespaces/newdb/tables/table", Map.of(), Map.of()),
+ reqMatcher(
+ HTTPMethod.HEAD,
+ "v1/namespaces/" + TABLE.namespace() + "/tables/" + TABLE.name(),
Review Comment:
but if we're going to fix this here, then we should fix this in the entire test class. You should also be able to replace this with `RESOURCE_PATHS.table(TABLE)`
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2636,7 +2636,11 @@ public void testTableExistsViaHEADRequest() {
any());
Mockito.verify(adapter)
.execute(
- reqMatcher(HTTPMethod.HEAD, "v1/namespaces/newdb/tables/table", Map.of(), Map.of()),
+ reqMatcher(
+ HTTPMethod.HEAD,
+ "v1/namespaces/" + TABLE.namespace() + "/tables/" + TABLE.name(),
Review Comment:
you should be able to replace this with `ResourcePaths.forCatalogProperties(Map.of()).table(TABLE)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #12768:
URL: https://github.com/apache/iceberg/pull/12768
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2044436773
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -884,7 +890,7 @@ public void testCreateTableWithDefaultColumnValue() {
catalog
.buildTable(ident, schemaWithDefault)
- .withLocation("file:/tmp/ns/table")
+ .withLocation(baseTableLocation(TBL))
Review Comment:
```suggestion
.withLocation(baseTableLocation(ident))
```
this one should be referring to the identifier local to the 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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2044443386
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1203,7 +1203,7 @@ public void testTableAuth(
Map<String, String> expectedContextHeaders,
Map<String, String> expectedTableHeaders,
String oauth2ServerUri) {
- TableIdentifier ident = TableIdentifier.of("ns", "table");
+ TableIdentifier ident = TableIdentifier.of("ns", "tbl");
Review Comment:
if we're introducing `TBL` then we should replace the local `ident` with `TBL`
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2044568223
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -908,7 +914,7 @@ public void testLoadTable() {
ImmutableMap.of("user", "someone", "created-at", "2022-02-25T00:38:19");
catalog
.buildTable(ident, SCHEMA)
- .withLocation("file:/tmp/ns/table")
+ .withLocation(baseTableLocation(TBL))
Review Comment:
Sam here. Now they all using TBL
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -908,7 +914,7 @@ public void testLoadTable() {
ImmutableMap.of("user", "someone", "created-at", "2022-02-25T00:38:19");
catalog
.buildTable(ident, SCHEMA)
- .withLocation("file:/tmp/ns/table")
+ .withLocation(baseTableLocation(TBL))
Review Comment:
Same here. Now they all using TBL
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2039119711
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -85,8 +85,9 @@
import org.junit.jupiter.params.provider.ValueSource;
public abstract class CatalogTests<C extends Catalog & SupportsNamespaces> {
+ private static final String DEFAULT_TABLE_LOCATION = "file:/tmp";
Review Comment:
```suggestion
private static final String BASE_TABLE_LOCATION = "file:/tmp";
```
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2042637930
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -191,6 +192,10 @@ protected boolean supportsEmptyNamespace() {
return false;
}
+ protected String baseTableLocation() {
Review Comment:
Make 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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2044572282
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1203,7 +1203,7 @@ public void testTableAuth(
Map<String, String> expectedContextHeaders,
Map<String, String> expectedTableHeaders,
String oauth2ServerUri) {
- TableIdentifier ident = TableIdentifier.of("ns", "table");
+ TableIdentifier ident = TableIdentifier.of("ns", "tbl");
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
Re: [PR] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#issuecomment-2795538333
@nastra Could you review 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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2044437466
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -908,7 +914,7 @@ public void testLoadTable() {
ImmutableMap.of("user", "someone", "created-at", "2022-02-25T00:38:19");
catalog
.buildTable(ident, SCHEMA)
- .withLocation("file:/tmp/ns/table")
+ .withLocation(baseTableLocation(TBL))
Review Comment:
```suggestion
.withLocation(baseTableLocation(ident))
```
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2044445636
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -538,7 +544,7 @@ public void testNamespaceWithDot() {
public void testBasicCreateTable() {
C catalog = catalog();
- TableIdentifier ident = TableIdentifier.of("ns", "table");
+ TableIdentifier ident = TableIdentifier.of("ns", "tbl");
Review Comment:
if we're introducing `TBL` then we should replace the local `ident` with `TBL` here and in all other test methods
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2045178435
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1256,37 +1255,37 @@ public void testTableAuth(
required(2, "data", Types.StringType.get()));
if (requiresNamespaceCreate()) {
- catalog.createNamespace(ident.namespace());
+ catalog.createNamespace(TBL.namespace());
}
- Table table = catalog.createTable(ident, expectedSchema);
+ Table table = catalog.createTable(TBL, expectedSchema);
assertThat(table.schema().asStruct())
.as("Schema should match")
.isEqualTo(expectedSchema.asStruct());
- Table loaded = catalog.loadTable(ident); // the first load will send the token
+ Table loaded = catalog.loadTable(TBL); // the first load will send the token
assertThat(loaded.schema().asStruct())
.as("Schema should match")
.isEqualTo(expectedSchema.asStruct());
loaded.refresh(); // refresh to force reload
- Mockito.verify(adapter)
+ verify(adapter)
Review Comment:
I was about to merge but just saw this and this seems like an unrelated change that we typically wouldn't want to include as part of this PR. Could you please revert the places that removed `Mockito`?
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2039122992
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -538,7 +543,7 @@ public void testNamespaceWithDot() {
public void testBasicCreateTable() {
C catalog = catalog();
- TableIdentifier ident = TableIdentifier.of("ns", "table");
+ TableIdentifier ident = TableIdentifier.of("ns", "nstable");
Review Comment:
nit: I'd probably then call this `tbl` instead of `nstable` because the `ns` in the same could be a bit misleading. Same for all the other places further below
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2039128672
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -1224,6 +1229,9 @@ public void testListTables() {
@Test
public void listNamespacesWithEmptyNamespace() {
+ assumeThat(supportsEmptyNamespace())
Review Comment:
I didn't add this on purpose because that behavior applies for all catalogs, independent whether they support empty namespaces or not. Is that not the case for your catalog impl?
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2041558611
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2686,7 +2690,7 @@ public <T extends RESTResponse> T execute(
.execute(
reqMatcher(
HTTPMethod.GET,
- "v1/namespaces/newdb/tables/table",
+ "v1/namespaces/" + TABLE.namespace() + "/tables/" + TABLE.name(),
Review Comment:
can be replaced with `RESOURCE_PATHS.table(TABLE)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2044567416
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -884,7 +890,7 @@ public void testCreateTableWithDefaultColumnValue() {
catalog
.buildTable(ident, schemaWithDefault)
- .withLocation("file:/tmp/ns/table")
+ .withLocation(baseTableLocation(TBL))
Review Comment:
I changed ident with TBL too
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2042566581
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2636,7 +2636,7 @@ public void testTableExistsViaHEADRequest() {
any());
Mockito.verify(adapter)
.execute(
- reqMatcher(HTTPMethod.HEAD, "v1/namespaces/newdb/tables/table", Map.of(), Map.of()),
+ reqMatcher(HTTPMethod.HEAD, RESOURCE_PATHS.table(TABLE), Map.of(), Map.of()),
Review Comment:
could you please update all the other places in this test class to using `RESOURCE_PATHS` instead of hardcoding the expected endpoint?
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2042637534
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2636,7 +2636,7 @@ public void testTableExistsViaHEADRequest() {
any());
Mockito.verify(adapter)
.execute(
- reqMatcher(HTTPMethod.HEAD, "v1/namespaces/newdb/tables/table", Map.of(), Map.of()),
+ reqMatcher(HTTPMethod.HEAD, RESOURCE_PATHS.table(TABLE), Map.of(), Map.of()),
Review Comment:
For sure. I changed the all places that I can see
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2041558611
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2686,7 +2690,7 @@ public <T extends RESTResponse> T execute(
.execute(
reqMatcher(
HTTPMethod.GET,
- "v1/namespaces/newdb/tables/table",
+ "v1/namespaces/" + TABLE.namespace() + "/tables/" + TABLE.name(),
Review Comment:
can be replaced with `ResourcePaths.forCatalogProperties(Map.of()).table(TABLE)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2039119129
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -191,6 +192,10 @@ protected boolean supportsEmptyNamespace() {
return false;
}
+ protected String testTableBaseLocation() {
Review Comment:
maybe name this `baseTableLocation()` instead
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2042544968
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -1224,6 +1229,9 @@ public void testListTables() {
@Test
public void listNamespacesWithEmptyNamespace() {
+ assumeThat(supportsEmptyNamespace())
Review Comment:
I am working on BigQuery Catalog. In the context of the BigQuery catalog, empty namespaces are not permitted as namespaces are a required component. I believe Snowflake has a similar requirement. I want to use CatalogTests for BigQuery Catalog. I believe we should check if Catalog support empty namespace support or not which I suspect is not supported by all catalog implementations.
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2042564996
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -2272,7 +2280,7 @@ public void testCompleteCreateOrReplaceTransactionCreate() {
if (!overridesRequestedLocation()) {
assertThat(table.location())
.as("Table location should match requested")
- .isEqualTo("file:/tmp/ns/table");
+ .isEqualTo(baseTableLocation() + "/" + TABLE.namespace() + "/" + TABLE.name());
Review Comment:
thinking more about this, maybe we should add this code into `baseTableLocation()` and update it to take the TableIdentifier as a parameter. The calling site would then just be `baseTableLocation(TABLE)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2042574912
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -191,6 +192,10 @@ protected boolean supportsEmptyNamespace() {
return false;
}
+ protected String baseTableLocation() {
Review Comment:
```suggestion
protected String baseTableLocation(TableIdentifier identifier) {
```
then we can move the string concatenation of the base location/namespace/table from all of the other places into this single place, which would make all calling sites simpler
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2042637091
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -2272,7 +2280,7 @@ public void testCompleteCreateOrReplaceTransactionCreate() {
if (!overridesRequestedLocation()) {
assertThat(table.location())
.as("Table location should match requested")
- .isEqualTo("file:/tmp/ns/table");
+ .isEqualTo(baseTableLocation() + "/" + TABLE.namespace() + "/" + TABLE.name());
Review Comment:
I changed the code accordingly
--
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] CatalogTests: Fix listNamespaces Check, Avoid Reserved Keyword, Allow Configurable Location [iceberg]
Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #12768:
URL: https://github.com/apache/iceberg/pull/12768#discussion_r2044561202
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -538,7 +544,7 @@ public void testNamespaceWithDot() {
public void testBasicCreateTable() {
C catalog = catalog();
- TableIdentifier ident = TableIdentifier.of("ns", "table");
+ TableIdentifier ident = TableIdentifier.of("ns", "tbl");
Review Comment:
Sounds good I did.
--
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