diqiu50 commented on code in PR #13722:
URL: https://github.com/apache/gravitino/pull/13722#discussion_r4398381021
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -231,10 +231,15 @@ public Table createTable(
}
}
+ // Validate registration before OVERWRITE can remove existing metadata or
managed data.
+ if (register) {
Review Comment:
In `CREATE` mode this validates before the existence check, so if the table
exists and the location is invalid, the caller gets 400 instead of 409. Maybe
validate up front only for `OVERWRITE`?
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -953,4 +958,50 @@ private IndexParams getIndexParamsByIndexType(IndexType
indexType) {
throw new IllegalArgumentException("Unsupported index type: " +
indexType);
}
}
+
+ private void dropExternalTableForRegistration(NameIdentifier ident) {
+ try {
+ Table existing = super.loadTable(ident);
+ validateLanceTable(ident, existing);
+ Preconditions.checkArgument(
+ LancePropertiesUtils.isExternalTable(existing.properties()),
+ "Cannot overwrite managed Lance table %s with a registration; "
+ + "explicitly drop or purge the managed table before registering
a replacement",
+ ident);
+ // Registration replaces external metadata only. Calling dropTable could
delete a managed
+ // dataset, including the same dataset that the incoming registration
just validated.
+ super.dropTable(ident);
+ } catch (NoSuchTableException e) {
+ // No existing registration needs replacing.
+ }
+ }
+
+ /**
+ * Validates that a caller-supplied location resolves to an existing, valid
Lance dataset before
+ * metadata changes adopt it. An explicit purge can later delete the
registered location, so a
+ * plain directory must not be adopted as if it were a Lance dataset (issue
#13696).
+ *
+ * <p>This checks dataset existence, not operator-defined storage scope or
purge authorization.
+ *
+ * @param ident the table being registered, used only for the error message
+ * @param location the caller-supplied dataset location
+ * @param properties the table properties, used to resolve storage options
+ * @throws IllegalArgumentException if dataset validation cannot complete
successfully
+ */
+ private void validateExistingDatasetLocation(
+ NameIdentifier ident, String location, Map<String, String> properties) {
+ Map<String, String> storageOptions =
+ LancePropertiesUtils.resolveLanceStorageOptions(catalogProperties,
properties);
+ try (Dataset ignored = openDataset(location, storageOptions)) {
+ // Opening a non-Lance location throws; reaching here confirms a valid
dataset.
+ LOG.debug("Validated Lance dataset at location {} for table {}",
location, ident);
+ } catch (Exception e) {
Review Comment:
This maps every failure, transient storage errors included, to
`IllegalArgumentException` (400). Could we use 400 only for missing or
non-Lance locations and wrap the other I/O errors with `ExceptionMessages.wrap`?
--
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]
