gh-yzou commented on code in PR #1368:
URL: https://github.com/apache/polaris/pull/1368#discussion_r2043283712
##########
plugins/spark/v3.5/src/main/java/org/apache/polaris/spark/PolarisSparkCatalog.java:
##########
@@ -90,12 +91,19 @@ public Table createTable(
@Override
public Table alterTable(Identifier identifier, TableChange... changes)
throws NoSuchTableException {
- throw new NoSuchTableException(identifier);
+ // alter table currently is not supported for general table formats
Review Comment:
udpated
##########
plugins/spark/v3.5/src/main/java/org/apache/polaris/spark/SparkCatalog.java:
##########
@@ -161,33 +163,59 @@ public Table createTable(
@Override
public Table alterTable(Identifier ident, TableChange... changes) throws
NoSuchTableException {
- throw new UnsupportedOperationException("alterTable");
+ try {
+ return this.icebergsSparkCatalog.alterTable(ident, changes);
+ } catch (NoSuchTableException e) {
+ Table table = this.polarisSparkCatalog.loadTable(ident);
+ String provider =
table.properties().get(PolarisCatalogUtils.TABLE_PROVIDER_KEY);
+ if (PolarisCatalogUtils.useDelta(provider)) {
+ // For delta table, most of the alter operations is a delta log
manipulation,
+ // we load the delta catalog to help handling the alter table
operation.
+ // NOTE: This currently doesn't work for changing file location and
file format
Review Comment:
yes, it does throw errors, the error comes directly from delta catalog and
spark,
for alter table set location, an error
[DELTA_CANNOT_SET_LOCATION_ON_PATH_IDENTIFIER] is thrown.
For alter table set fileformat, there is actually an error "Operation not
allowed"
If the delta catalog need to call the delegate.alterTable underneath, we
directly throw an unsupported operations error.
I didn't find a good way to test this in unit test, but i will definitely
include those scenario in the integration tests
##########
plugins/spark/v3.5/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java:
##########
@@ -138,12 +152,51 @@ public void close() throws IOException {
@Override
public List<TableIdentifier> listGenericTables(Namespace ns) {
- throw new UnsupportedOperationException("listTables not supported");
+ if (!endpoints.contains(PolarisEndpoints.V1_LIST_GENERIC_TABLES)) {
+ return ImmutableList.of();
+ }
+
+ Map<String, String> queryParams = Maps.newHashMap();
+ ImmutableList.Builder<TableIdentifier> tables = ImmutableList.builder();
+ String pageToken = "";
+ if (pageSize != null) {
+ queryParams.put("pageSize", String.valueOf(pageSize));
+ }
+
+ do {
+ queryParams.put("pageToken", pageToken);
+ ListTablesResponse response =
+ restClient
+ .withAuthSession(this.catalogAuth)
+ .get(
+ pathGenerator.genericTables(ns),
+ queryParams,
+ ListTablesResponse.class,
+ Map.of(),
+ ErrorHandlers.namespaceErrorHandler());
+ pageToken = response.nextPageToken();
+ tables.addAll(response.identifiers());
+ } while (pageToken != null);
+
+ return tables.build();
}
@Override
public boolean dropGenericTable(TableIdentifier identifier) {
- throw new UnsupportedOperationException("dropTable not supported");
+ Endpoint.check(endpoints, PolarisEndpoints.V1_DELETE_GENERIC_TABLE);
+
+ try {
+ restClient
+ .withAuthSession(this.catalogAuth)
+ .delete(
+ pathGenerator.genericTable(identifier),
+ null,
+ Map.of(),
+ ErrorHandlers.tableErrorHandler());
+ return true;
+ } catch (NoSuchTableException e) {
+ return false;
Review Comment:
This is actually the same as iceberg here
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L307
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
