You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "eric-maynard (via GitHub)" <gi...@apache.org> on 2025/04/03 04:52:06 UTC
[PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
eric-maynard opened a new pull request, #1298:
URL: https://github.com/apache/polaris/pull/1298
A followup to #1264, this PR implements `GenericTableCatalogAdapter` to make generic table methods accessible through the REST API. During manual testing, I noticed some issues with grants and some inconsistent error messages related to generic tables so those are addressed here as well.
Tests and enablement of generic table APIs by default will follow #1286 and the availability of a client.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #1298:
URL: https://github.com/apache/polaris/pull/1298#issuecomment-2777590516
@gh-yzou
> ... there should be some setup to have raw RESTClient that can be used to call the service
Is there? I think for the admin service we use `PolarisAdminServiceAuthzTest` and then also the integration/regression tests
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028088176
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1495,10 +1490,10 @@ public boolean grantPrivilegeOnTableToRole(
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.ADD_TABLE_GRANT_TO_CATALOG_ROLE;
authorizeGrantOnTableLikeOperationOrThrow(
- op, catalogName, PolarisEntitySubType.ICEBERG_TABLE, identifier, catalogRoleName);
+ op, catalogName, PolarisEntitySubType.ANY_SUBTYPE, identifier, catalogRoleName);
return grantPrivilegeOnTableLikeToRole(
- catalogName, catalogRoleName, identifier, PolarisEntitySubType.ICEBERG_TABLE, privilege);
+ catalogName, catalogRoleName, identifier, PolarisEntitySubType.ANY_SUBTYPE, privilege);
Review Comment:
Also, note that there is test coverage for these error messages in Iceberg (for Iceberg tables and views)
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2029949193
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -366,4 +349,24 @@ protected void authorizeRenameTableLikeOperationOrThrow(
initializeCatalog();
}
+
+ /**
+ * Helper function for when a TABLE_LIKE entity is not found so we want to throw the appropriate
+ * exception. Used in Iceberg APIs, so the Iceberg messages cannot be changed.
+ *
+ * @param subType The subtype of the entity that the exception should report doesn't exist
+ */
+ public static void throwNotFoundException(
+ TableIdentifier identifier, PolarisEntitySubType subType) {
+ switch (subType) {
+ case ICEBERG_TABLE:
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ case ICEBERG_VIEW:
+ throw new NoSuchViewException("View does not exist: %s", identifier);
+ case GENERIC_TABLE:
+ throw new NoSuchTableException("Generic table does not exist: %s", identifier);
+ default:
+ throw new NoSuchTableException("Entity does not exist: %s", subType);
Review Comment:
I think that's a better message, and with the method renamed that's probably more clear. However I am a little worried that we can't differentiate from the `ICEBERG_TABLE` branch in that case
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2030614778
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/GenericTableApi.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.it.env;
+
+import static jakarta.ws.rs.core.Response.Status.NO_CONTENT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.Entity;
+import jakarta.ws.rs.core.MultivaluedHashMap;
+import jakarta.ws.rs.core.Response;
+import java.net.URI;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.responses.OAuthTokenResponse;
+import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+
+/**
+ * A simple, non-exhaustive set of helper methods for accessing the generic tables REST API
+ *
+ * @see PolarisClient#genericTableApi(ClientCredentials)
+ */
+public class GenericTableApi extends RestApi {
+ GenericTableApi(Client client, PolarisApiEndpoints endpoints, String authToken, URI uri) {
+ super(client, endpoints, authToken, uri);
+ }
+
+ public String obtainToken(ClientCredentials credentials) {
+ try (Response response =
+ request("v1/oauth/tokens")
+ .post(
+ Entity.form(
+ new MultivaluedHashMap<>(
+ Map.of(
+ "grant_type",
+ "client_credentials",
+ "scope",
+ "PRINCIPAL_ROLE:ALL",
+ "client_id",
+ credentials.clientId(),
+ "client_secret",
+ credentials.clientSecret()))))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ return response.readEntity(OAuthTokenResponse.class).token();
+ }
+ }
+
+ public void purge(String catalog, Namespace ns) {
+ listGenericTables(catalog, ns).forEach(t -> dropGenericTable(catalog, t));
+ }
+
+ public List<TableIdentifier> listGenericTables(String catalog, Namespace namespace) {
+ String ns = RESTUtil.encodeNamespace(namespace);
+ try (Response res =
+ request("polaris/v1/{cat}/namespaces/{ns}/generic-tables", Map.of("cat", catalog, "ns", ns))
Review Comment:
This is the same pattern I found in [CatalogApi](https://github.com/apache/polaris/blob/ee0d0083e513c56f5ef1d4282bb15ae1f6153b54/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java#L165), which seems fine for a test. This way, if the paths change the rest will show a regression even if `PolarisResourcePaths` is also changed.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2026293089
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -474,8 +474,10 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
+ } else if (subType == PolarisEntitySubType.ICEBERG_VIEW) {
throw new NoSuchViewException("View does not exist: %s", identifier);
+ } else if (subType == PolarisEntitySubType.GENERIC_TABLE) {
+ throw new NoSuchViewException("Generic table does not exist: %s", identifier);
Review Comment:
```suggestion
throw new NoSuchTableException("Generic table does not exist: %s", identifier);
```
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -474,8 +474,10 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
+ } else if (subType == PolarisEntitySubType.ICEBERG_VIEW) {
throw new NoSuchViewException("View does not exist: %s", identifier);
+ } else if (subType == PolarisEntitySubType.GENERIC_TABLE) {
Review Comment:
can just put it as `else` or if there can any other value, lets throw IllegalStateException ?
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028094941
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -356,4 +348,23 @@ protected void authorizeRenameTableLikeOperationOrThrow(
initializeCatalog();
}
+
+ /**
+ * Helper function for when a TABLE_LIKE entity is not found & we want to throw the appropriate
+ * exception
+ *
+ * @param subType The subtype of the entity that the exception should report doesn't exist
+ */
+ public static void throwNotFoundException(
Review Comment:
That kind of makes it sound like it's throwing an exception called `TableLikeEntityNotFoundException`, no?
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028084926
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
`subType` is never ANY_SUBTYPE 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028095738
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
But if we want to change the message, then I think the existing check can work with a slight change to the `else` condition. Ideally we would pass in the subtype here, but that would require refactors to `TableGrant` which could be a bit unnecessarily invasive just for an error message. Let me try that way 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #1298:
URL: https://github.com/apache/polaris/pull/1298#issuecomment-2774785185
@singhpk234
> are we ok with iceberg views referencing generic tables ?
Good question! Right now there isn't a canonical way to reference generic tables in SQL; in the design the Spark client will just always check if a table is a generic table after confirming it's neither a view nor an Iceberg table. In such a case, I think it's reasonable to allow Iceberg views to refer to generic tables.
In another implementation where generic tables are accessed another way (e.g. via UDTF) then we might want to revisit this decision.
What happens in Spark today if you try to create an Iceberg view that doesn't include an Iceberg table, but it reads from a UDTF? Does it work?
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2031677847
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogAdapter.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.common;
+
+import java.net.URLEncoder;
+import java.nio.charset.Charset;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.rest.RESTUtil;
+
+/**
+ * A common interface for adapters between the REST interface and {@link CatalogHandler}
+ * implementations
+ */
+public interface CatalogAdapter {
Review Comment:
nit: the decodeNamespace seems a very simple utility function, do we need an interface for this? maybe we can have a common utility class, and put this under it.
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/GenericTableApi.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.it.env;
+
+import static jakarta.ws.rs.core.Response.Status.NO_CONTENT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.Entity;
+import jakarta.ws.rs.core.MultivaluedHashMap;
+import jakarta.ws.rs.core.Response;
+import java.net.URI;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.responses.OAuthTokenResponse;
+import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+
+/**
+ * A simple, non-exhaustive set of helper methods for accessing the generic tables REST API
+ *
+ * @see PolarisClient#genericTableApi(ClientCredentials)
+ */
+public class GenericTableApi extends RestApi {
+ GenericTableApi(Client client, PolarisApiEndpoints endpoints, String authToken, URI uri) {
+ super(client, endpoints, authToken, uri);
+ }
+
+ public String obtainToken(ClientCredentials credentials) {
+ try (Response response =
+ request("v1/oauth/tokens")
+ .post(
+ Entity.form(
+ new MultivaluedHashMap<>(
+ Map.of(
+ "grant_type",
+ "client_credentials",
+ "scope",
+ "PRINCIPAL_ROLE:ALL",
+ "client_id",
+ credentials.clientId(),
+ "client_secret",
+ credentials.clientSecret()))))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ return response.readEntity(OAuthTokenResponse.class).token();
+ }
+ }
+
+ public void purge(String catalog, Namespace ns) {
+ listGenericTables(catalog, ns).forEach(t -> dropGenericTable(catalog, t));
+ }
+
+ public List<TableIdentifier> listGenericTables(String catalog, Namespace namespace) {
+ String ns = RESTUtil.encodeNamespace(namespace);
+ try (Response res =
+ request("polaris/v1/{cat}/namespaces/{ns}/generic-tables", Map.of("cat", catalog, "ns", ns))
Review Comment:
Yeah, i think it is fine to have this for 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2036245026
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/GenericTableApi.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.it.env;
+
+import static jakarta.ws.rs.core.Response.Status.NO_CONTENT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.Entity;
+import jakarta.ws.rs.core.MultivaluedHashMap;
+import jakarta.ws.rs.core.Response;
+import java.net.URI;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.responses.OAuthTokenResponse;
+import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+
+/**
+ * A simple, non-exhaustive set of helper methods for accessing the generic tables REST API
+ *
+ * @see PolarisClient#genericTableApi(ClientCredentials)
+ */
+public class GenericTableApi extends RestApi {
+ GenericTableApi(Client client, PolarisApiEndpoints endpoints, String authToken, URI uri) {
+ super(client, endpoints, authToken, uri);
+ }
+
+ public String obtainToken(ClientCredentials credentials) {
Review Comment:
Minor: do we need this method? I don't think we need to test the token endpoint here.
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java:
##########
@@ -1194,4 +1203,143 @@ public void testLoadCredentials() {
assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
}
}
+
+ @Test
+ public void testCreateGenericTable() {
+ Namespace namespace = Namespace.of("ns1");
+ restCatalog.createNamespace(namespace);
+ TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "tbl1");
+
+ GenericTable createResponse =
+ genericTableApi.createGenericTable(currentCatalogName, tableIdentifier, "format", Map.of());
+ Assertions.assertThat(createResponse.getFormat()).isEqualTo("format");
+
+ genericTableApi.purge(currentCatalogName, namespace);
+ }
+
+ @Test
+ public void testLoadGenericTable() {
+ Namespace namespace = Namespace.of("ns1");
+ restCatalog.createNamespace(namespace);
+ TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "tbl1");
+
+ genericTableApi.createGenericTable(currentCatalogName, tableIdentifier, "format", Map.of());
+
+ GenericTable loadResponse =
+ genericTableApi.getGenericTable(currentCatalogName, tableIdentifier);
+ Assertions.assertThat(loadResponse.getFormat()).isEqualTo("format");
+
+ genericTableApi.purge(currentCatalogName, namespace);
+ }
+
+ @Test
+ public void testListGenericTables() {
+ Namespace namespace = Namespace.of("ns1");
+ restCatalog.createNamespace(namespace);
+ TableIdentifier tableIdentifier1 = TableIdentifier.of(namespace, "tbl1");
+ TableIdentifier tableIdentifier2 = TableIdentifier.of(namespace, "tbl2");
+
+ genericTableApi.createGenericTable(currentCatalogName, tableIdentifier1, "format", Map.of());
+ genericTableApi.createGenericTable(currentCatalogName, tableIdentifier2, "format", Map.of());
+
+ List<TableIdentifier> identifiers =
+ genericTableApi.listGenericTables(currentCatalogName, namespace);
+
+ Assertions.assertThat(identifiers).hasSize(2);
+ Assertions.assertThat(identifiers)
+ .containsExactlyInAnyOrder(tableIdentifier1, tableIdentifier2);
+
+ genericTableApi.purge(currentCatalogName, namespace);
+ }
+
+ @Test
+ public void testDropGenericTable() {
+ Namespace namespace = Namespace.of("ns1");
+ restCatalog.createNamespace(namespace);
+ TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "tbl1");
+
+ genericTableApi.createGenericTable(currentCatalogName, tableIdentifier, "format", Map.of());
+
+ GenericTable loadResponse1 =
Review Comment:
nit: probably just `loadResponse`
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogAdapter.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.common;
+
+import java.net.URLEncoder;
+import java.nio.charset.Charset;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.rest.RESTUtil;
+
+/**
+ * A common interface for adapters between the REST interface and {@link CatalogHandler}
+ * implementations
+ */
+public interface CatalogAdapter {
Review Comment:
I'd also prefer to move it to a common util class,
1. It isn't needed in the inheritance chain.
2. GenericTable, iceberg, policy all need it as a common static method.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -366,4 +349,24 @@ protected void authorizeRenameTableLikeOperationOrThrow(
initializeCatalog();
}
+
+ /**
+ * Helper function for when a TABLE_LIKE entity is not found so we want to throw the appropriate
+ * exception. Used in Iceberg APIs, so the Iceberg messages cannot be changed.
+ *
+ * @param subType The subtype of the entity that the exception should report doesn't exist
+ */
+ public static void throwNotFoundExceptionForTableLikeEntity(
+ TableIdentifier identifier, PolarisEntitySubType subType) {
+ switch (subType) {
+ case ICEBERG_TABLE:
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ case ICEBERG_VIEW:
+ throw new NoSuchViewException("View does not exist: %s", identifier);
+ case GENERIC_TABLE:
+ throw new NoSuchTableException("Generic table does not exist: %s", identifier);
+ default:
+ throw new NoSuchTableException("Table does not exist: %s", subType);
Review Comment:
Minor: something like this?
```suggestion
throw new NoSuchTableException("Table with type %s does not exist: %s", subType, identifier);
```
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -468,22 +467,30 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
catalogRoleName);
ResolverStatus status = resolutionManifest.resolveAll();
+ final PolarisEntitySubType searchSubtype;
+ if (subTypes.size() > 1) {
+ searchSubtype = PolarisEntitySubType.ANY_SUBTYPE;
+ } else {
+ searchSubtype = subTypes.getFirst();
+ }
+
if (status.getStatus() == ResolverStatus.StatusEnum.ENTITY_COULD_NOT_BE_RESOLVED) {
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundExceptionForTableLikeEntity(identifier, searchSubtype);
} else {
throw new NotFoundException("CatalogRole not found: %s.%s", catalogName, catalogRoleName);
}
}
PolarisResolvedPathWrapper tableLikeWrapper =
- resolutionManifest.getResolvedPath(identifier, PolarisEntityType.TABLE_LIKE, subType, true);
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, searchSubtype, true);
Review Comment:
I'd recommend to add a override of `resolutionManifest.getResolvedPath()` to take a list of subtypes.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogAdapter.java:
##########
@@ -19,22 +19,81 @@
package org.apache.polaris.service.catalog.generic;
import jakarta.enterprise.context.RequestScoped;
+import jakarta.inject.Inject;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
+import org.apache.polaris.core.auth.PolarisAuthorizer;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.persistence.PolarisEntityManager;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.service.catalog.api.PolarisCatalogGenericTableApiService;
+import org.apache.polaris.service.catalog.common.CatalogAdapter;
import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@RequestScoped
-public class GenericTableCatalogAdapter implements PolarisCatalogGenericTableApiService {
+public class GenericTableCatalogAdapter
+ implements PolarisCatalogGenericTableApiService, CatalogAdapter {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(GenericTableCatalogAdapter.class);
+
+ private final CallContext callContext;
+ private final PolarisEntityManager entityManager;
+ private final PolarisMetaStoreManager metaStoreManager;
+ private final PolarisAuthorizer polarisAuthorizer;
+
+ @Inject
+ public GenericTableCatalogAdapter(
+ CallContext callContext,
+ PolarisEntityManager entityManager,
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisAuthorizer polarisAuthorizer) {
+ this.callContext = callContext;
+ this.entityManager = entityManager;
+ this.metaStoreManager = metaStoreManager;
+ this.polarisAuthorizer = polarisAuthorizer;
+ }
+
+ private GenericTableCatalogHandler newHandlerWrapper(
+ SecurityContext securityContext, String catalogName) {
+ AuthenticatedPolarisPrincipal authenticatedPrincipal =
+ (AuthenticatedPolarisPrincipal) securityContext.getUserPrincipal();
Review Comment:
nit: we could keep them in one line if we use `var`
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2030614778
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/GenericTableApi.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.it.env;
+
+import static jakarta.ws.rs.core.Response.Status.NO_CONTENT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.Entity;
+import jakarta.ws.rs.core.MultivaluedHashMap;
+import jakarta.ws.rs.core.Response;
+import java.net.URI;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.responses.OAuthTokenResponse;
+import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+
+/**
+ * A simple, non-exhaustive set of helper methods for accessing the generic tables REST API
+ *
+ * @see PolarisClient#genericTableApi(ClientCredentials)
+ */
+public class GenericTableApi extends RestApi {
+ GenericTableApi(Client client, PolarisApiEndpoints endpoints, String authToken, URI uri) {
+ super(client, endpoints, authToken, uri);
+ }
+
+ public String obtainToken(ClientCredentials credentials) {
+ try (Response response =
+ request("v1/oauth/tokens")
+ .post(
+ Entity.form(
+ new MultivaluedHashMap<>(
+ Map.of(
+ "grant_type",
+ "client_credentials",
+ "scope",
+ "PRINCIPAL_ROLE:ALL",
+ "client_id",
+ credentials.clientId(),
+ "client_secret",
+ credentials.clientSecret()))))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ return response.readEntity(OAuthTokenResponse.class).token();
+ }
+ }
+
+ public void purge(String catalog, Namespace ns) {
+ listGenericTables(catalog, ns).forEach(t -> dropGenericTable(catalog, t));
+ }
+
+ public List<TableIdentifier> listGenericTables(String catalog, Namespace namespace) {
+ String ns = RESTUtil.encodeNamespace(namespace);
+ try (Response res =
+ request("polaris/v1/{cat}/namespaces/{ns}/generic-tables", Map.of("cat", catalog, "ns", ns))
Review Comment:
This is the same pattern I found in [CatalogApi](https://github.com/apache/polaris/blob/ee0d0083e513c56f5ef1d4282bb15ae1f6153b54/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java#L165), which seems fine for a test. This way, if the paths change the test will show a regression even if `PolarisResourcePaths` is also changed.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028090363
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
> when isTable is True, we can throw, no Iceberg or Generic Table is found.
At least for the catalog methods, we need to exactly match the error messages expected by Iceberg. More generally, it's not a great idea to change these messages if we can help it. If ANY_SUBTYPE is never passed in here (and it shouldn't be) the messages are correct
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
But if we want to change the message, then I think the existing check can work with a slight change to the `else` condition. Ideally we would pass in the subtype here, but that would require refactors to `TableGrant` which could be a bit unnecessarily invasive just for an error message. Let me try that way 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028111630
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1495,10 +1490,10 @@ public boolean grantPrivilegeOnTableToRole(
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.ADD_TABLE_GRANT_TO_CATALOG_ROLE;
authorizeGrantOnTableLikeOperationOrThrow(
- op, catalogName, PolarisEntitySubType.ICEBERG_TABLE, identifier, catalogRoleName);
+ op, catalogName, PolarisEntitySubType.ANY_SUBTYPE, identifier, catalogRoleName);
return grantPrivilegeOnTableLikeToRole(
- catalogName, catalogRoleName, identifier, PolarisEntitySubType.ICEBERG_TABLE, privilege);
+ catalogName, catalogRoleName, identifier, PolarisEntitySubType.ANY_SUBTYPE, privilege);
Review Comment:
Since this subtype is only used for errors like you point out above, it's actually fine to keep it as ICEBERG_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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard merged PR #1298:
URL: https://github.com/apache/polaris/pull/1298
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #1298:
URL: https://github.com/apache/polaris/pull/1298#issuecomment-2776220792
> What happens in Spark today if you try to create an Iceberg view that doesn't include an Iceberg table, but it reads from a UDTF? Does it work?
It works right now, but heads up community is thinking of restricting this to iceberg table only, and using table uuid, to have consistent view resolution.
we should discuss this a bit more on the view resolution context, Not urgent or blocker for 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028090363
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
> when isTable is True, we can throw, no Iceberg or Generic Table is found.
At least for the catalog methods, we need to exactly match the error messages expected by Iceberg. More generally, it's not a great idea to change these messages if we can help it
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1495,10 +1490,10 @@ public boolean grantPrivilegeOnTableToRole(
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.ADD_TABLE_GRANT_TO_CATALOG_ROLE;
authorizeGrantOnTableLikeOperationOrThrow(
- op, catalogName, PolarisEntitySubType.ICEBERG_TABLE, identifier, catalogRoleName);
+ op, catalogName, PolarisEntitySubType.ANY_SUBTYPE, identifier, catalogRoleName);
return grantPrivilegeOnTableLikeToRole(
- catalogName, catalogRoleName, identifier, PolarisEntitySubType.ICEBERG_TABLE, privilege);
+ catalogName, catalogRoleName, identifier, PolarisEntitySubType.ANY_SUBTYPE, privilege);
Review Comment:
Also, note that there is test coverage for these error messages in Iceberg (for Iceberg tables and views)
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2029494686
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/GenericTableApi.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.it.env;
+
+import static jakarta.ws.rs.core.Response.Status.NO_CONTENT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.Entity;
+import jakarta.ws.rs.core.MultivaluedHashMap;
+import jakarta.ws.rs.core.Response;
+import java.net.URI;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.responses.OAuthTokenResponse;
+import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+
+/**
+ * A simple, non-exhaustive set of helper methods for accessing the generic tables REST API
+ *
+ * @see PolarisClient#genericTableApi(ClientCredentials)
+ */
+public class GenericTableApi extends RestApi {
+ GenericTableApi(Client client, PolarisApiEndpoints endpoints, String authToken, URI uri) {
+ super(client, endpoints, authToken, uri);
+ }
+
+ public String obtainToken(ClientCredentials credentials) {
+ try (Response response =
+ request("v1/oauth/tokens")
+ .post(
+ Entity.form(
+ new MultivaluedHashMap<>(
+ Map.of(
+ "grant_type",
+ "client_credentials",
+ "scope",
+ "PRINCIPAL_ROLE:ALL",
+ "client_id",
+ credentials.clientId(),
+ "client_secret",
+ credentials.clientSecret()))))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ return response.readEntity(OAuthTokenResponse.class).token();
+ }
+ }
+
+ public void purge(String catalog, Namespace ns) {
+ listGenericTables(catalog, ns).forEach(t -> dropGenericTable(catalog, t));
+ }
+
+ public List<TableIdentifier> listGenericTables(String catalog, Namespace namespace) {
+ String ns = RESTUtil.encodeNamespace(namespace);
+ try (Response res =
+ request("polaris/v1/{cat}/namespaces/{ns}/generic-tables", Map.of("cat", catalog, "ns", ns))
Review Comment:
you can init a PolarisResourcePaths instance with catalog and then directly call genericTables methods to get the request path, this is also how client used to generate the path also, then we don't have to hard code the path.
```
PolarisResourcePaths paths = new PolarisResourcePaths(catalog);
request(paths.genericTables(ns));
```
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -366,4 +349,24 @@ protected void authorizeRenameTableLikeOperationOrThrow(
initializeCatalog();
}
+
+ /**
+ * Helper function for when a TABLE_LIKE entity is not found so we want to throw the appropriate
+ * exception. Used in Iceberg APIs, so the Iceberg messages cannot be changed.
+ *
+ * @param subType The subtype of the entity that the exception should report doesn't exist
+ */
+ public static void throwNotFoundException(
+ TableIdentifier identifier, PolarisEntitySubType subType) {
+ switch (subType) {
+ case ICEBERG_TABLE:
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ case ICEBERG_VIEW:
+ throw new NoSuchViewException("View does not exist: %s", identifier);
+ case GENERIC_TABLE:
+ throw new NoSuchTableException("Generic table does not exist: %s", identifier);
+ default:
+ throw new NoSuchTableException("Entity does not exist: %s", subType);
Review Comment:
How about update this message "Entity does not exist" -> "Table does not exist" to give a more clear message to user. Right now, seems ANY_SUBTYPE is used when we are handling table in general List.of(PolarisEntitySubType.GENERIC_TABLE, PolarisEntitySubType.ICEBERG_TABLE). Then it is clear to the user that it is the Table doesn't exist
##########
service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogAdapter.java:
##########
@@ -19,22 +19,86 @@
package org.apache.polaris.service.catalog.generic;
import jakarta.enterprise.context.RequestScoped;
+import jakarta.inject.Inject;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
+import org.apache.polaris.core.auth.PolarisAuthorizer;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.persistence.PolarisEntityManager;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.service.catalog.api.PolarisCatalogGenericTableApiService;
import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@RequestScoped
public class GenericTableCatalogAdapter implements PolarisCatalogGenericTableApiService {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(GenericTableCatalogAdapter.class);
+
+ private final CallContext callContext;
+ private final PolarisEntityManager entityManager;
+ private final PolarisMetaStoreManager metaStoreManager;
+ private final PolarisAuthorizer polarisAuthorizer;
+
+ @Inject
+ public GenericTableCatalogAdapter(
+ CallContext callContext,
+ PolarisEntityManager entityManager,
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisAuthorizer polarisAuthorizer) {
+ this.callContext = callContext;
+ this.entityManager = entityManager;
+ this.metaStoreManager = metaStoreManager;
+ this.polarisAuthorizer = polarisAuthorizer;
+ }
+
+ private Namespace rawStringToNamespace(String namespace) {
Review Comment:
The function there is just a direct call to RESTUtil.decodeNamespace, and RESTUtil is coming from iceberg rest, so we are using the same decode methods as iceberg. if we want to make it more clear, we can directly call RESTUtil.decodeNamespace in the code, so we are consistent on how the namespace is decoded.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -356,4 +348,23 @@ protected void authorizeRenameTableLikeOperationOrThrow(
initializeCatalog();
}
+
+ /**
+ * Helper function for when a TABLE_LIKE entity is not found & we want to throw the appropriate
+ * exception
+ *
+ * @param subType The subtype of the entity that the exception should report doesn't exist
+ */
+ public static void throwNotFoundException(
Review Comment:
I was making this suggestion because this function is specifically designed for TableLike entities, not for NamespaceEntities or other type of Entities. The function name throwNotFoundException is very general, more sounds like for all possible entities. So I was suggesting to have *TableLike* in the name to help make the function name more clear. Another options is throwNotFoundExceptionForTabeLike etc.
However, I see the restriction is stated in the comment also, so that is a nit suggestion.
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
The admin API function is our internal function, that kind of interface change seems fine to me. Made a small suggestion to make the message more clear.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028089017
##########
service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogAdapter.java:
##########
@@ -19,22 +19,86 @@
package org.apache.polaris.service.catalog.generic;
import jakarta.enterprise.context.RequestScoped;
+import jakarta.inject.Inject;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
+import org.apache.polaris.core.auth.PolarisAuthorizer;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.persistence.PolarisEntityManager;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.service.catalog.api.PolarisCatalogGenericTableApiService;
import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@RequestScoped
public class GenericTableCatalogAdapter implements PolarisCatalogGenericTableApiService {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(GenericTableCatalogAdapter.class);
+
+ private final CallContext callContext;
+ private final PolarisEntityManager entityManager;
+ private final PolarisMetaStoreManager metaStoreManager;
+ private final PolarisAuthorizer polarisAuthorizer;
+
+ @Inject
+ public GenericTableCatalogAdapter(
+ CallContext callContext,
+ PolarisEntityManager entityManager,
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisAuthorizer polarisAuthorizer) {
+ this.callContext = callContext;
+ this.entityManager = entityManager;
+ this.metaStoreManager = metaStoreManager;
+ this.polarisAuthorizer = polarisAuthorizer;
+ }
+
+ private Namespace rawStringToNamespace(String namespace) {
Review Comment:
Don't we use Iceberg code to do this on the Iceberg side? I don't think it would be possible to share utilities
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028084926
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
`subType` is never ANY_SUBTYPE 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028094941
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -356,4 +348,23 @@ protected void authorizeRenameTableLikeOperationOrThrow(
initializeCatalog();
}
+
+ /**
+ * Helper function for when a TABLE_LIKE entity is not found & we want to throw the appropriate
+ * exception
+ *
+ * @param subType The subtype of the entity that the exception should report doesn't exist
+ */
+ public static void throwNotFoundException(
Review Comment:
That kind of makes it sound like it's throwing an exception called `TableLikeEntityNotFoundException`, no? I'm mostly thinking about the catalog APIs here, we are throwing Iceberg exceptions like `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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2027940930
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1495,10 +1490,10 @@ public boolean grantPrivilegeOnTableToRole(
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.ADD_TABLE_GRANT_TO_CATALOG_ROLE;
authorizeGrantOnTableLikeOperationOrThrow(
- op, catalogName, PolarisEntitySubType.ICEBERG_TABLE, identifier, catalogRoleName);
+ op, catalogName, PolarisEntitySubType.ANY_SUBTYPE, identifier, catalogRoleName);
return grantPrivilegeOnTableLikeToRole(
- catalogName, catalogRoleName, identifier, PolarisEntitySubType.ICEBERG_TABLE, privilege);
+ catalogName, catalogRoleName, identifier, PolarisEntitySubType.ANY_SUBTYPE, privilege);
Review Comment:
i think we can do similar thing for grantPrivilegeOnTableLikeToRole also
##########
service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogAdapter.java:
##########
@@ -19,22 +19,86 @@
package org.apache.polaris.service.catalog.generic;
import jakarta.enterprise.context.RequestScoped;
+import jakarta.inject.Inject;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
+import org.apache.polaris.core.auth.PolarisAuthorizer;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.persistence.PolarisEntityManager;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.service.catalog.api.PolarisCatalogGenericTableApiService;
import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@RequestScoped
public class GenericTableCatalogAdapter implements PolarisCatalogGenericTableApiService {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(GenericTableCatalogAdapter.class);
+
+ private final CallContext callContext;
+ private final PolarisEntityManager entityManager;
+ private final PolarisMetaStoreManager metaStoreManager;
+ private final PolarisAuthorizer polarisAuthorizer;
+
+ @Inject
+ public GenericTableCatalogAdapter(
+ CallContext callContext,
+ PolarisEntityManager entityManager,
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisAuthorizer polarisAuthorizer) {
+ this.callContext = callContext;
+ this.entityManager = entityManager;
+ this.metaStoreManager = metaStoreManager;
+ this.polarisAuthorizer = polarisAuthorizer;
+ }
+
+ private Namespace rawStringToNamespace(String namespace) {
Review Comment:
How about move this function to a util class https://github.com/apache/polaris/blob/main/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java#L235, and then reuse the same decode function for all namespaces, we are using the same namespace concept across Polaris, it would be better if we share the same namespace uitlities
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -356,4 +348,23 @@ protected void authorizeRenameTableLikeOperationOrThrow(
initializeCatalog();
}
+
+ /**
+ * Helper function for when a TABLE_LIKE entity is not found & we want to throw the appropriate
+ * exception
+ *
+ * @param subType The subtype of the entity that the exception should report doesn't exist
+ */
+ public static void throwNotFoundException(
Review Comment:
nit : Maybe we can make the name of this function more clear, like throwTableLikeEntityNotFoundException
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
Also, we might need to do a verification after line 480 to make sure the tableLikeEntity we found is table or view based on the isTable configuration.
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
I see the subtype is passed as PolarisEntitySubType.ANY_SUBTYPE here, and based on how the function is implemented, it seems we will just thrown
```
throw new IllegalStateException("Unrecognized entity subtype " + subType);
```
So the error message may not give the correct information here.
I took a look of the function, the subtype here doesn't seem doing much here. I think we can simply update the subType parameter to boolean isTable like following:
```
private void authorizeGrantOnTableLikeOperationOrThrow(
PolarisAuthorizableOperation op,
String catalogName,
boolean isTable,
TableIdentifier identifier,
String catalogRoleName)
```
when isTable is True, we can throw, no Iceberg or Generic Table is found.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2026180746
##########
service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogHandler.java:
##########
@@ -77,7 +77,7 @@ public ListGenericTablesResponse listGenericTables(Namespace parent) {
authorizeBasicNamespaceOperationOrThrow(op, parent);
return ListGenericTablesResponse.builder()
- .setIdentifiers(new TreeSet<>(genericTableCatalog.listGenericTables(parent)))
+ .setIdentifiers(new LinkedHashSet<>(genericTableCatalog.listGenericTables(parent)))
Review Comment:
We should prefer to preserve the existing order, not necessarily to sort by name
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2031695706
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogAdapter.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.common;
+
+import java.net.URLEncoder;
+import java.nio.charset.Charset;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.rest.RESTUtil;
+
+/**
+ * A common interface for adapters between the REST interface and {@link CatalogHandler}
+ * implementations
+ */
+public interface CatalogAdapter {
Review Comment:
If we need it somewhere else we should factor it out. Previously it lived in `IcebergCatalog` so this seems like a good place for it after the 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2030614778
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/GenericTableApi.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.it.env;
+
+import static jakarta.ws.rs.core.Response.Status.NO_CONTENT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.Entity;
+import jakarta.ws.rs.core.MultivaluedHashMap;
+import jakarta.ws.rs.core.Response;
+import java.net.URI;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.responses.OAuthTokenResponse;
+import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+
+/**
+ * A simple, non-exhaustive set of helper methods for accessing the generic tables REST API
+ *
+ * @see PolarisClient#genericTableApi(ClientCredentials)
+ */
+public class GenericTableApi extends RestApi {
+ GenericTableApi(Client client, PolarisApiEndpoints endpoints, String authToken, URI uri) {
+ super(client, endpoints, authToken, uri);
+ }
+
+ public String obtainToken(ClientCredentials credentials) {
+ try (Response response =
+ request("v1/oauth/tokens")
+ .post(
+ Entity.form(
+ new MultivaluedHashMap<>(
+ Map.of(
+ "grant_type",
+ "client_credentials",
+ "scope",
+ "PRINCIPAL_ROLE:ALL",
+ "client_id",
+ credentials.clientId(),
+ "client_secret",
+ credentials.clientSecret()))))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ return response.readEntity(OAuthTokenResponse.class).token();
+ }
+ }
+
+ public void purge(String catalog, Namespace ns) {
+ listGenericTables(catalog, ns).forEach(t -> dropGenericTable(catalog, t));
+ }
+
+ public List<TableIdentifier> listGenericTables(String catalog, Namespace namespace) {
+ String ns = RESTUtil.encodeNamespace(namespace);
+ try (Response res =
+ request("polaris/v1/{cat}/namespaces/{ns}/generic-tables", Map.of("cat", catalog, "ns", ns))
Review Comment:
This is the same pattern I found in [CatalogApi](https://github.com/apache/polaris/blob/ee0d0083e513c56f5ef1d4282bb15ae1f6153b54/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java#L165), which seems fine for a 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2028084926
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
`subtType` is never ANY_SUBTYPE 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement GenericTableCatalogAdapter; admin-related fixes [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2029210457
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() == ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() == PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s", identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
I've changed the method a bit to return a reasonable error for ANY_SUBTYPE, the error messages should be good now. They will unfortunately change a bit for the admin API though. Is this 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org