Copilot commented on code in PR #11070:
URL: https://github.com/apache/gravitino/pull/11070#discussion_r3232409528
##########
docs/lakehouse-generic-lance-table.md:
##########
@@ -318,15 +318,12 @@ curl -X POST -H "Accept:
application/vnd.gravitino.v1+json" \
"nullable": false
}
],
- "properties": {
+ "properties": {
"format": "lance",
- "location": "s3://bucket1/lance_orders",
- "lance.storage.access_key_id": "ak",
- "lance.storage.endpoint": "http://minio:9000";,
- "lance.storage.secret_access_key": "sk",
- "lance.storage.allow_http": "true"
- }
+ "location": "s3://bucket1/lance_orders"
+ }
Review Comment:
The JSON example formatting/indentation is inconsistent: `format`/`location`
should be clearly nested under `properties`, and the closing brace indentation
should match. While whitespace doesn’t change JSON validity, this is much
harder to read and easy to copy incorrectly—please reformat the example with
consistent indentation.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -242,6 +247,13 @@ public Table createTable(
Map<String, String> newProperties = Maps.newHashMap(properties);
newProperties.put(Table.PROPERTY_LOCATION, tableLocation);
newProperties.put(Table.PROPERTY_TABLE_FORMAT, format);
+ if ("lance".equals(format)) {
+ LancePropertiesUtils.getLanceStorageOptions(catalogProperties)
+ .forEach(
+ (key, value) ->
+ newProperties.putIfAbsent(
+ LanceConstants.LANCE_STORAGE_OPTIONS_PREFIX + key,
value));
+ }
Review Comment:
This copies catalog-level `lance.storage.*` values into persisted table
properties. If catalog properties include credentials (e.g.,
`secret_access_key`), this duplicates and persists secrets at the table level,
increasing exposure via metadata access and backups, and it partially
undermines the stated goal of keeping Gravitino as the source of truth. Prefer
resolving catalog defaults at request/runtime (as you already do in Lance REST)
without materializing them into table properties; if table-level persistence is
required for Lance engine internals, consider persisting only non-sensitive
fields, storing credentials in a secret manager/credential provider, or marking
credential keys as hidden/reserved and ensuring they are not returned via
metadata APIs.
##########
docs/lance-rest-integration.md:
##########
@@ -126,7 +126,7 @@ spark.sql("SELECT * FROM sales.orders").show()
The `LOCATION` clause in the `CREATE TABLE` statement is optional. When
omitted, lance-spark automatically determines an appropriate storage location
based on catalog properties.
For detailed information on location resolution logic, refer to the [Lakehouse
Generic Catalog
documentation](./lakehouse-generic-catalog.md#key-property-location).
-For cloud storage backends such as Amazon S3 or MinIO, specify credentials and
endpoint configuration in the table properties:
+For Gravitino-managed Lance catalogs, put the storage configuration in the
Gravitino catalog properties so Spark does not need to repeat it.
Review Comment:
The updated guidance says to configure storage at the Gravitino catalog
level, but the section no longer shows *how* to set those `lance.storage.*`
catalog properties (REST example / SQL example / link to the exact
catalog-properties section). Adding a minimal catalog-creation example (or a
direct link to the catalog property configuration docs/endpoint) would make
this actionable and reduce user confusion.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -95,6 +95,10 @@ public class LanceRESTServiceIT extends BaseIT {
private static final String CATALOG_NAME =
GravitinoITUtils.genRandomName("lance_rest_catalog");
private static final String SCHEMA_NAME =
GravitinoITUtils.genRandomName("lance_rest_schema");
private static final String DELIMITER = ".";
+ private static final String MINIO_ENDPOINT = "http://127.0.0.1:9000";;
+ private static final String MINIO_REGION = "us-east-1";
+ private static final String MINIO_ACCESS_KEY = "minioadmin";
+ private static final String MINIO_SECRET_KEY = "minioadmin";
Review Comment:
These MinIO constants are duplicated in multiple IT classes in this PR.
Consider extracting them into a shared integration-test utility (or the
`BaseIT` if appropriate) to reduce drift and make future updates (e.g.,
endpoint/creds) less error-prone.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -242,6 +247,13 @@ public Table createTable(
Map<String, String> newProperties = Maps.newHashMap(properties);
newProperties.put(Table.PROPERTY_LOCATION, tableLocation);
newProperties.put(Table.PROPERTY_TABLE_FORMAT, format);
+ if ("lance".equals(format)) {
Review Comment:
Using a raw string literal for the table format makes this branch sensitive
to casing/normalization differences and harder to refactor safely. If a
table-format constant/enum exists in this module, prefer that; otherwise
consider normalizing (e.g., lowercasing once) so `Lance`/`LANCE` inputs don’t
silently skip catalog-default propagation.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -661,6 +665,43 @@ void testCreateTable() throws IOException {
.contains("Column non_existing_column does not exist in the
dataset"));
}
+ @Test
+ void testCreateTableUsesCatalogStorageOptions() throws IOException {
+ catalog =
createCatalog(GravitinoITUtils.genRandomName("lance_rest_catalog"));
+ createSchema();
+
+ String location = tempDir + "/" + "catalog_storage_table/";
Review Comment:
Building filesystem paths via string concatenation can be platform-dependent
and can produce double separators. Prefer constructing the location using
`Path` operations (e.g., `tempDir.resolve(...)`) and then converting to string,
which is more robust and consistent across environments.
--
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]
