bharos commented on code in PR #10586:
URL: https://github.com/apache/gravitino/pull/10586#discussion_r3042847132
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -239,7 +265,7 @@ public LoadTableResponse updateTable(
metadataCache.invalidate(tableIdentifier);
LoadTableResponse loadTableResponse =
CatalogHandlers.updateTable(catalog, tableIdentifier,
updateTableRequest);
- if (loadTableResponse != null) {
+ if (loadTableResponse != null && !(catalog instanceof RESTCatalog)) {
Review Comment:
I think this is still needed to be fixed ?
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -535,4 +617,111 @@ private static void replaceDeprecatedProperties(
properties.put(newProperty, deprecatedValue);
}
}
+
+ private LoadTableResponse createTableInternal(Namespace namespace,
CreateTableRequest request) {
+
+ request.validate();
+
+ if (request.stageCreate()) {
+ return stageTableCreateInternal(namespace, request);
+ }
+
+ TableIdentifier ident = TableIdentifier.of(namespace, request.name());
+ Table table =
+ catalog
+ .buildTable(ident, request.schema())
+ .withLocation(request.location())
+ .withPartitionSpec(request.spec())
+ .withSortOrder(request.writeOrder())
+ .withProperties(request.properties())
+ .create();
+
+ if (table instanceof BaseTable) {
+ Map<String, String> properties = retrieveFileIOProperties(table.io());
+ return LoadTableResponse.builder()
+ .withTableMetadata(((BaseTable) table).operations().current())
+ .addAllConfig(
+ MapUtils.getFilteredMap(
+ properties, key ->
catalogPropertiesToClientKeys.contains(key)))
+ // Keep only credential fields from FileIO properties before
returning them to the client.
+
.addAllConfig(CredentialPropertyUtils.filterCredentialProperties(properties))
+ .build();
+ }
+
+ throw new IllegalStateException("Cannot wrap catalog that does not produce
BaseTable");
+ }
+
+ private LoadTableResponse stageTableCreateInternal(
+ Namespace namespace, CreateTableRequest request) {
+ if (!(catalog instanceof RESTCatalog)) {
Review Comment:
stageTableCreateInternal() is only called from createTableInternal(), which
itself is only entered when catalog instanceof RESTCatalog.
So should we just remove this check code
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -96,12 +105,18 @@ public class CatalogWrapperForREST extends
IcebergCatalogWrapper {
"gcs-credential-file-path",
GCSProperties.GRAVITINO_GCS_SERVICE_ACCOUNT_FILE);
+ protected static final Set<String> catalogPropertiesToClientKeys =
Review Comment:
This was private earlier , still can keep private ?
--
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]
