shaoyu-li commented on code in PR #13058:
URL: https://github.com/apache/gravitino/pull/13058#discussion_r4008535097
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -233,25 +269,38 @@ public Table createTable(
Index[] indexes)
throws NoSuchSchemaException, TableAlreadyExistsException {
Schema schema = loadSchema(NameIdentifier.of(ident.namespace().levels()));
- String tableLocation = calculateTableLocation(schema, ident, properties);
String format = properties.getOrDefault(Table.PROPERTY_TABLE_FORMAT, null);
Preconditions.checkArgument(
format != null, "Table format must be specified in table properties");
format = format.toLowerCase(Locale.ROOT);
- Map<String, String> newProperties = Maps.newHashMap(properties);
- newProperties.put(Table.PROPERTY_LOCATION, tableLocation);
- newProperties.put(Table.PROPERTY_TABLE_FORMAT, format);
-
// Get the table operations for the specified table format.
Supplier<ManagedTableOperations> tableOpsSupplier =
tableOpsCache.get(format);
Preconditions.checkArgument(tableOpsSupplier != null, "Unsupported table
format: %s", format);
ManagedTableOperations tableOps =
configureTableOps(tableOpsSupplier.get());
+ // The provider is consulted only once the request is known to be one this
catalog can serve,
+ // and only when the catalog is the one choosing the location. A provider
that allocates real
+ // storage has no compensating callback, so every check that can be made
before asking it for a
+ // location is one reservation it does not have to reclaim later.
+ String suppliedLocation = properties.get(Table.PROPERTY_LOCATION);
+ boolean provisioned = StringUtils.isBlank(suppliedLocation);
Review Comment:
You're right, and the dead-branch observation is what settles it: if the
catalog resolves a supplied
location on the provider's behalf, the first branch of
`DefaultTableLocationProvider` can never run,
which is the code saying the same thing your argument does.
Changed in 8ffd3c2. `provisionTableLocation` is now called for every
creation the catalog can serve,
with the supplied value visible as the `location` entry of the context's
table properties. The
provider honours it, rewrites it, or throws. A catalog on the built-in
provider is unaffected --
returning it verbatim is that first branch, and a test pins that an external
and a managed table
both come out with the same value, trailing slash included, as before.
The contract now says plainly what an allocating provider has to do with
that case, since the risk
the old rule was protecting against is real and just moves to the provider:
a Lance registration or
an external Delta table points at data that already exists, and returning a
fresh path for one of
those orphans it while the creation still reports success.
Two things fall out. The `provisioned` special case in `createTable` is
gone, so
`releaseUnusedLocation` now applies uniformly. And one of the two
asymmetries I listed under the
external rule -- "a non-external table whose creation carried its own
location was never
provisioned, yet is still unprovisioned" -- disappears, because it is
provisioned now.
--
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]
