mchades commented on code in PR #9463:
URL: https://github.com/apache/gravitino/pull/9463#discussion_r2610017755
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -259,8 +260,15 @@ Table createTableInternal(
throws NoSuchSchemaException, TableAlreadyExistsException {
if (register) {
+ // If this is a register operation, it should be a external table.
+ Map<String, String> stringStringMap = Maps.newHashMap(properties);
+ if (!properties.containsKey(Table.PROPERTY_EXTERNAL)
+ || !Boolean.parseBoolean(properties.get(Table.PROPERTY_EXTERNAL))) {
+ stringStringMap.put(Table.PROPERTY_EXTERNAL, "true");
+ }
Review Comment:
I remember JackYe said that the expectation for `registerTable` is that the
location must exist, so I think when using `register`, if the location does not
exist, we can directly throw an exception to reject the register (in this case,
the user should be using `createEmptyTable`).
> "register" and "external" are two different behavior definitions; we
should not use one definition to restrict another. The user can register an
existing table and let Gravitino manage the metadata and data.
Agreed.
The `external` is a table property introduced by Gravitino for Lance format
table, used to differentiate the behavior during drop table (whether to delete
storage data). It should not be linked with the `register` property, and it
should be up to the user to decide whether to register the table as `external`
or `managed`. If the user does not specify, then the default value of
`external` being `false` also makes sense, as we expect the table to be fully
managed by Gravitino.
--
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]
