shaoyu-li commented on code in PR #13058:
URL: https://github.com/apache/gravitino/pull/13058#discussion_r4008550188
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -129,22 +132,35 @@ protected EntityStore store() {
public void initialize(
Map<String, String> conf, CatalogInfo info, HasPropertyMetadata
propertiesMetadata)
throws RuntimeException {
- this.catalogProperties = conf == null ? Map.of() : Maps.newHashMap(conf);
- String location =
+ // A defensive copy that tolerates null values, for the same reason as
+ // TableLocationContext.Builder#withTableProperties: nothing upstream
rejects a catalog
+ // property whose value is null, and ImmutableMap.copyOf would turn one
into a
+ // NullPointerException. Here it would fail the creation of the whole
catalog rather than a
+ // single request.
+ this.catalogProperties =
+ conf == null ? Map.of() :
Collections.unmodifiableMap(Maps.newHashMap(conf));
+
+ String providerName =
(String)
propertiesMetadata
.catalogPropertiesMetadata()
- .getOrDefault(conf, Catalog.PROPERTY_LOCATION);
- this.catalogLocation =
- StringUtils.isNotBlank(location)
- ? Optional.of(location).map(this::ensureTrailingSlash)
- : Optional.empty();
- this.propertiesMetadata = propertiesMetadata;
+ .getOrDefault(conf,
GenericCatalogPropertiesMetadata.TABLE_LOCATION_PROVIDER);
Review Comment:
Both correct, and your framing of why is the right one -- the half-way state
is what misleads,
because the test passes against the fake delegator while the real format
fails. Finished rather than
withdrawn, in 8ffd3c2:
1. `table-location-provider` present with a null value now falls back to
`default` via
`StringUtils.defaultIfBlank`, which is the same answer as leaving the
property out -- what a null
value means. `testANullProviderNameFallsBackToTheBuiltInProvider` pins it
and fails without the
fallback.
2. `LanceTableOperations.setCatalogProperties` makes the same null-tolerant
copy the generic catalog
does, instead of `ImmutableMap.copyOf`.
--
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]
