Copilot commented on code in PR #11070:
URL: https://github.com/apache/gravitino/pull/11070#discussion_r3238898092
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogPropertiesMetadata.java:
##########
@@ -41,7 +43,14 @@ public class GenericCatalogPropertiesMetadata extends
BaseCatalogPropertiesMetad
"The root directory of the generic catalog.",
false /* immutable */,
null, /* defaultValue */
- false /* hidden */));
+ false /* hidden */),
+ stringOptionalPropertyPrefixEntry(
+ LANCE_STORAGE_OPTIONS_PREFIX,
+ "The Lance storage options managed by the catalog.",
+ false /* immutable */,
+ null, /* defaultValue */
+ false /* hidden */,
Review Comment:
This prefix makes every `lance.storage.*` catalog property public because it
is marked `hidden=false`. Since the examples and tests put
`lance.storage.secret_access_key` under this prefix, `Catalog.properties()` and
Lance `describeNamespace` will expose the secret as ordinary catalog metadata;
other credential catalog properties in the codebase are hidden (for example
Glue AWS keys). Please avoid exposing secret storage options through
catalog/namespace describe while still making them available to the authorized
Lance table path.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -360,4 +364,12 @@ private ManagedTableOperations tableOps(NameIdentifier
tableIdent) {
}
}
}
+
+ private ManagedTableOperations configureTableOps(ManagedTableOperations ops)
{
+ if (ops instanceof LanceTableOperations) {
+ ((LanceTableOperations) ops).setCatalogProperties(catalogProperties);
+ }
Review Comment:
This hard-codes a Lance-specific `instanceof` into the generic catalog
operation path, bypassing the `LakehouseTableDelegator` abstraction that is
meant to supply format-specific operations. If another delegator or a
wrapped/custom table operation needs catalog-level context, it will silently
miss it unless this method is edited again; consider a small interface for
table ops that consume catalog properties instead of checking the concrete
Lance class here.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -277,7 +293,8 @@ Table createTableInternal(
ident, columns, comment, properties, partitions, distribution,
sortOrders, indexes);
}
- Map<String, String> storageProps =
LancePropertiesUtils.getLanceStorageOptions(properties);
+ Map<String, String> storageProps =
+ LancePropertiesUtils.resolveLanceStorageOptions(catalogProperties,
properties);
Review Comment:
The new catalog-default path in `LanceTableOperations` is not covered by the
existing `TestLanceTableOperations` suite; the added REST IT only asserts
response maps and uses a local temp path, so it would not catch failures
passing catalog S3/MinIO options into `Dataset.write`/drop. Please add a
focused test around `setCatalogProperties` and the effective storage options
used by Lance operations.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -100,6 +102,16 @@ protected IdGenerator idGenerator() {
return idGenerator;
}
+ /**
+ * Sets the catalog properties used to resolve Lance storage defaults at
runtime.
+ *
+ * @param catalogProperties the catalog properties
+ */
+ public void setCatalogProperties(Map<String, String> catalogProperties) {
+ this.catalogProperties =
+ catalogProperties == null ? Map.of() :
ImmutableMap.copyOf(catalogProperties);
+ }
Review Comment:
Catalog defaults are only applied in the create/drop paths;
`handleLanceTableChange` still opens the dataset without resolving these
catalog properties. A table that relies on catalog-level S3/MinIO credentials
can therefore be created successfully but later fail on column/index
alterations because the alter path has no storage options.
--
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]
