Copilot commented on code in PR #11473:
URL: https://github.com/apache/gravitino/pull/11473#discussion_r3370635381
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -134,7 +134,8 @@ public void initialize(
Map<String, String> config, CatalogInfo info, HasPropertyMetadata
propertiesMetadata)
throws RuntimeException {
this.glueClient = GlueClientProvider.buildClient(config);
- this.catalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID);
+ String rawCatalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID);
+ this.catalogId = (rawCatalogId != null && !rawCatalogId.isEmpty()) ?
rawCatalogId : null;
Review Comment:
This behavior change (treating empty catalog ID as unset) is not covered by
the existing unit tests in this module: current tests either omit the property
or set `ops.catalogId` directly. Please add a unit test that sets
`aws-glue-catalog-id` to "" (and ideally whitespace) and asserts it is not
propagated into Glue request builders / Iceberg Glue catalog props.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -134,7 +134,8 @@ public void initialize(
Map<String, String> config, CatalogInfo info, HasPropertyMetadata
propertiesMetadata)
throws RuntimeException {
this.glueClient = GlueClientProvider.buildClient(config);
- this.catalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID);
+ String rawCatalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID);
+ this.catalogId = (rawCatalogId != null && !rawCatalogId.isEmpty()) ?
rawCatalogId : null;
Review Comment:
Catalog ID normalization only checks `isEmpty()`, so whitespace-only values
(e.g., " ") will still be treated as a real catalog ID and propagated. This
module generally treats blank config values as unset (e.g.,
`GlueClientProvider` uses `StringUtils.isNotBlank`), so this should be
consistent here too.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergTableHelper.java:
##########
@@ -157,7 +157,7 @@ static Catalog createGlueCatalog(Map<String, String>
config) {
icebergProps.put(IcebergConstants.AWS_S3_REGION, region);
String catalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID);
- if (catalogId != null) {
+ if (catalogId != null && !catalogId.isEmpty()) {
icebergProps.put(GLUE_ID, catalogId);
}
Review Comment:
Same as in `GlueCatalogOperations.initialize`, this should ignore
blank/whitespace-only catalog IDs (not just the empty string). Otherwise a
value like " " still gets propagated into Iceberg's GlueCatalog properties.
--
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]
