eric-maynard commented on code in PR #1439: URL: https://github.com/apache/polaris/pull/1439#discussion_r2057600965
########## service/common/src/main/java/org/apache/polaris/service/catalog/conversion/xtable/RunSyncRequest.java: ########## @@ -0,0 +1,50 @@ +/* + * 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.conversion.xtable; + +import com.fasterxml.jackson.annotation.JsonProperty; + +public class RunSyncRequest { Review Comment: These types should reside in `polaris-core` or `polaris-service-common` and we should provide an xtable implementation for them like we do for the metastore types & eclipselink ########## service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java: ########## @@ -331,7 +337,17 @@ public LoadTableResponse createTableDirect(Namespace namespace, CreateTableReque if (isStaticFacade(catalog)) { throw new BadRequestException("Cannot create table on static-facade external catalogs."); } - return CatalogHandlers.createTable(baseCatalog, namespace, request); + // TODO check to see if we will need do this in createTableWriteDelegation, createTableStaged, Review Comment: Yes, we do. Also, this needs to be feature flagged off. ########## polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java: ########## @@ -209,6 +209,20 @@ public static void enforceFeatureEnabledOrThrow( .defaultValue(true) .buildFeatureConfiguration(); + public static final FeatureConfiguration<Boolean> ENABLE_XTABLE_REST_SERVICE = + PolarisConfiguration.<Boolean>builder() + .key("ENABLE_XTABLE_REST_SERVICE") + .description( + "If true, delegates table metadata conversion to an external XTable REST Service") Review Comment: Why would the Polaris catalog service have a feature flag that controls whether or not the XTable service is "enabled" (running)? We _do_ need feature flags to control a few behaviors: 1. Convert-on-write 2. Convert-on-read ########## polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java: ########## @@ -209,6 +209,20 @@ public static void enforceFeatureEnabledOrThrow( .defaultValue(true) .buildFeatureConfiguration(); + public static final FeatureConfiguration<Boolean> ENABLE_XTABLE_REST_SERVICE = + PolarisConfiguration.<Boolean>builder() + .key("ENABLE_XTABLE_REST_SERVICE") + .description( + "If true, delegates table metadata conversion to an external XTable REST Service") Review Comment: However we implement this, it should probably live in application.properties instead of being a feature configuration checked at runtime. It also needs to allow different conversion implementations to be specified. So something like: ``` table-conversion.type = xtable table-conversion.host=www.my-xtable.com table.conversion.supported-types=delta,hudi,iceberg ``` or: ``` table-conversion.type=local table.conversion.supported-types=delta,iceberg ``` Check out how the `RealmContextResolver` is configured for example. ########## service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java: ########## @@ -587,7 +604,17 @@ public Optional<LoadTableResponse> loadTableIfStale( } LoadTableResponse rawResponse = CatalogHandlers.loadTable(baseCatalog, tableIdentifier); - return Optional.of(filterResponseToSnapshots(rawResponse, snapshots)); + Optional<LoadTableResponse> optionalLoadTableResponse = Optional.of(filterResponseToSnapshots(rawResponse, snapshots)); + if (tableEntity != null Review Comment: This needs to be feature flagged off. Also, this only handls conversion to Iceberg. To handle conversion to other formats, you'll need to look at `GenericTableCatalogHandler`. ########## service/common/src/main/java/org/apache/polaris/service/catalog/conversion/xtable/TableFormat.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.conversion.xtable; + +import com.google.common.base.Preconditions; +import java.util.Locale; + +public enum TableFormat { Review Comment: This enum probably doesn't belong to the xtable conversion implementation -- it's either something that relates to the `format` of a `GenericTable`, or possibly something that relates to conversion more generally -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
