fix: store Lance tables in HMS as external tables

docs/src/hive2.md

@@ -28,7 +28,7 @@ The **namespace identifier** is the database name.
 
 ### Table
 
-A **table** is represented as a [Table object](https://github.com/apache/hive/blob/branch-2.3/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Table.java) in HMS with `tableType` set to `EXTERNAL_TABLE`.
+A **table** is represented as a [Table object](https://github.com/apache/hive/blob/branch-2.3/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Table.java) in HMS with `tableType` set to `EXTERNAL_TABLE` and the table parameter `EXTERNAL=TRUE`. Both must be set, because HMS silently stores a table declared as `EXTERNAL_TABLE` without the `EXTERNAL=TRUE` parameter as a `MANAGED_TABLE`.
 
 The **table identifier** is constructed by joining database and table name with the `$` delimiter (e.g., `database$table`).
 
@@ -38,7 +38,7 @@ The **table location** is stored in the `location` field of the table's `storage
 
 ## Lance Table Identification
 
-A table in HMS is identified as a Lance table when it meets the following criteria: the `tableType` is `EXTERNAL_TABLE`, and the `parameters` map contains a key `table_type` with value `lance` (case insensitive). The `location` in `storageDescriptor` may be declared before a Lance dataset exists; storage is checked only for `include_declared=false` listing or `check_declared=true` describe requests.
+A table in HMS is identified as a Lance table when the `parameters` map contains a key `table_type` with value `lance` (case insensitive). The `location` in `storageDescriptor` may be declared before a Lance dataset exists; storage is checked only for `include_declared=false` listing or `check_declared=true` describe requests.
 
 ## Basic Operations
 
@@ -116,7 +116,7 @@ The implementation:
 2. Verify the parent namespace exists
 3. Create an HMS Table object with `tableType=EXTERNAL_TABLE`
 4. Set the storage descriptor with the specified or default location. When location is not specified, it defaults to `{root}/{database}.db/{table}`
-5. Merge request `properties` with required table parameters such as `table_type=lance` and `managed_by=storage`
+5. Merge request `properties` with required table parameters such as `table_type=lance`, `managed_by=storage`, and `EXTERNAL=TRUE`
 6. Register the table in HMS
 7. Return the declared table location, table parameters, and `managed_versioning=false`
 
@@ -175,7 +175,8 @@ The implementation:
 
 1. Parse the table identifier
 2. Retrieve the Table object and validate it is a Lance table
-3. Drop the table from HMS with `deleteData=true`, which removes both the metadata and the underlying Lance table data
+3. Drop the table from HMS with `deleteData=false`, since HMS does not delete data for external tables
+4. Delete the underlying Lance dataset at the table location as a best-effort cleanup
 
 **Error Handling:**
 

docs/src/hive3.md

@@ -28,7 +28,7 @@ The **namespace identifier** is constructed by joining namespace levels with the
 
 ### Table
 
-A **table** is represented as a [Table object](https://github.com/apache/hive/blob/branch-4.0/standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift#L631) in HMS with `tableType` set to `EXTERNAL_TABLE`.
+A **table** is represented as a [Table object](https://github.com/apache/hive/blob/branch-4.0/standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift#L631) in HMS with `tableType` set to `EXTERNAL_TABLE` and the table parameter `EXTERNAL=TRUE`. Both must be set, because HMS silently stores a table declared as `EXTERNAL_TABLE` without the `EXTERNAL=TRUE` parameter as a `MANAGED_TABLE`.
 
 The **table identifier** is constructed by joining catalog, database, and table name with the `$` delimiter (e.g., `catalog$database$table`).
 
@@ -38,7 +38,7 @@ The **table location** is stored in the [`location`](https://github.com/apache/h
 
 ## Lance Table Identification
 
-A table in HMS is identified as a Lance table when it meets the following criteria: the `tableType` is `EXTERNAL_TABLE`, and the `parameters` map contains a key `table_type` with value `lance` (case insensitive). The `location` in `storageDescriptor` may be declared before a Lance dataset exists; storage is checked only for `include_declared=false` listing or `check_declared=true` describe requests.
+A table in HMS is identified as a Lance table when the `parameters` map contains a key `table_type` with value `lance` (case insensitive). The `location` in `storageDescriptor` may be declared before a Lance dataset exists; storage is checked only for `include_declared=false` listing or `check_declared=true` describe requests.
 
 ## Basic Operations
 
@@ -123,7 +123,7 @@ The implementation:
 2. Verify the parent namespace exists
 3. Create an HMS Table object with `tableType=EXTERNAL_TABLE`
 4. Set the storage descriptor with the specified or default location. When location is not specified, it defaults to `{root}/{database}.db/{table}` for the default `hive` catalog (hive2-compatible), or `{root}/{catalog}/{database}.db/{table}` for other catalogs
-5. Merge request `properties` with required table parameters such as `table_type=lance` and `managed_by=storage`
+5. Merge request `properties` with required table parameters such as `table_type=lance`, `managed_by=storage`, and `EXTERNAL=TRUE`
 6. Register the table in HMS
 7. Return the declared table location, table parameters, and `managed_versioning=false`
 
@@ -182,7 +182,8 @@ The implementation:
 
 1. Parse the table identifier
 2. Retrieve the Table object and validate it is a Lance table
-3. Drop the table from HMS with `deleteData=true`, which removes both the metadata and the underlying Lance table data
+3. Drop the table from HMS with `deleteData=false`, since HMS does not delete data for external tables
+4. Delete the underlying Lance dataset at the table location as a best-effort cleanup
 
 **Error Handling:**
 

java/lance-namespace-hive2/src/main/java/org/lance/namespace/hive2/Hive2Namespace.java

@@ -503,6 +503,8 @@ protected void doCreateTable(
       table.setSd(sd);
 
       Map<String, String> params = Hive2Util.createLanceTableParams(properties);
+      // HMS silently rewrites the table type to MANAGED_TABLE unless this parameter is set
+      params.put("EXTERNAL", "TRUE");
       table.setParameters(params);
 
       clientPool.run(
@@ -586,12 +588,18 @@ protected String doDropTable(ObjectIdentifier id, boolean deleteData) {
       Hive2Util.validateLanceTable(hmsTable.get());
       String location = hmsTable.get().getSd().getLocation();
 
+      // HMS does not delete data for external tables, so drop metadata only and delete the
+      // Lance dataset explicitly when requested
       clientPool.run(
           client -> {
-            client.dropTable(db, tableName, deleteData, true /* ignoreUnknownTable */);
+            client.dropTable(db, tableName, false /* deleteData */, true /* ignoreUnknownTable */);
             return null;
           });
 
+      if (deleteData) {
+        safeDropDataset(location);
+      }
+
       return location;
     } catch (TException | InterruptedException e) {
       if (e instanceof InterruptedException) {
@@ -603,6 +611,14 @@ protected String doDropTable(ObjectIdentifier id, boolean deleteData) {
     }
   }
 
+  private void safeDropDataset(String location) {
+    try {
+      Dataset.drop(location, Collections.emptyMap());
+    } catch (Exception e) {
+      LOG.warn("Failed to delete Lance dataset at {} for dropped table", location, e);
+    }
+  }
+
   protected Map<String, String> doDropNamespace(ObjectIdentifier id, String mode, String behavior) {
     String db = id.levelAtListPos(0).toLowerCase();
 

java/lance-namespace-hive2/src/test/java/org/lance/namespace/hive2/TestHive2Namespace.java

@@ -13,14 +13,19 @@
  */
 package org.lance.namespace.hive2;
 
+import org.lance.Dataset;
+import org.lance.WriteParams;
 import org.lance.namespace.LanceNamespace;
 import org.lance.namespace.errors.LanceNamespaceException;
 import org.lance.namespace.model.CreateNamespaceRequest;
+import org.lance.namespace.model.DeclareTableRequest;
+import org.lance.namespace.model.DeregisterTableRequest;
 import org.lance.namespace.model.DescribeNamespaceRequest;
 import org.lance.namespace.model.DescribeNamespaceResponse;
 import org.lance.namespace.model.DescribeTableRequest;
 import org.lance.namespace.model.DropNamespaceRequest;
 import org.lance.namespace.model.DropNamespaceResponse;
+import org.lance.namespace.model.DropTableRequest;
 import org.lance.namespace.model.ListTablesRequest;
 import org.lance.namespace.model.ListTablesResponse;
 import org.lance.namespace.model.NamespaceExistsRequest;
@@ -29,7 +34,11 @@
 import com.google.common.collect.Maps;
 import org.apache.arrow.memory.BufferAllocator;
 import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.vector.types.pojo.ArrowType;
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.arrow.vector.types.pojo.Schema;
 import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.Table;
 import org.assertj.core.util.Lists;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.AfterEach;
@@ -38,12 +47,14 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.util.Collections;
 import java.util.Map;
 
 import static java.nio.file.Files.createTempDirectory;
 import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute;
 import static java.nio.file.attribute.PosixFilePermissions.fromString;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -277,6 +288,104 @@ public void testDropNamespaceFailMode() {
     assertTrue(error.getMessage().contains("Database non_existent_db doesn't exist"));
   }
 
+  @Test
+  public void testDeclareTableIsExternalInHms() throws Exception {
+    CreateNamespaceRequest nsRequest = new CreateNamespaceRequest();
+    nsRequest.setId(Lists.list("test_db"));
+    nsRequest.setMode("Create");
+    namespace.createNamespace(nsRequest);
+
+    DeclareTableRequest declareRequest = new DeclareTableRequest();
+    declareRequest.setId(Lists.list("test_db", "ext_table"));
+    declareRequest.setLocation(tmpDirBase + "/ext_table");
+    namespace.declareTable(declareRequest);
+
+    Table hmsTable = metastore.clientPool().run(client -> client.getTable("test_db", "ext_table"));
+    assertEquals("EXTERNAL_TABLE", hmsTable.getTableType());
+    assertEquals("TRUE", hmsTable.getParameters().get("EXTERNAL"));
+  }
+
+  @Test
+  public void testDropTableDeletesData() throws Exception {
+    CreateNamespaceRequest nsRequest = new CreateNamespaceRequest();
+    nsRequest.setId(Lists.list("test_db"));
+    nsRequest.setMode("Create");
+    namespace.createNamespace(nsRequest);
+
+    String location = tmpDirBase + "/drop_data_table";
+    DeclareTableRequest declareRequest = new DeclareTableRequest();
+    declareRequest.setId(Lists.list("test_db", "drop_data_table"));
+    declareRequest.setLocation(location);
+    namespace.declareTable(declareRequest);
+
+    Schema schema =
+        new Schema(Collections.singletonList(Field.nullable("id", new ArrowType.Int(32, true))));
+    WriteParams writeParams =
+        new WriteParams.Builder().withMode(WriteParams.WriteMode.CREATE).build();
+    Dataset.create(allocator, location, schema, writeParams).close();
+    assertTrue(new File(location).exists());
+
+    DropTableRequest dropRequest = new DropTableRequest();
+    dropRequest.setId(Lists.list("test_db", "drop_data_table"));
+    namespace.dropTable(dropRequest);
+
+    TableExistsRequest existsRequest = new TableExistsRequest();
+    existsRequest.setId(Lists.list("test_db", "drop_data_table"));
+    assertThrows(LanceNamespaceException.class, () -> namespace.tableExists(existsRequest));
+    assertFalse(new File(location).exists());
+  }
+
+  @Test
+  public void testDropTableDeclaredOnly() {
+    CreateNamespaceRequest nsRequest = new CreateNamespaceRequest();
+    nsRequest.setId(Lists.list("test_db"));
+    nsRequest.setMode("Create");
+    namespace.createNamespace(nsRequest);
+
+    DeclareTableRequest declareRequest = new DeclareTableRequest();
+    declareRequest.setId(Lists.list("test_db", "declared_only_table"));
+    declareRequest.setLocation(tmpDirBase + "/declared_only_table");
+    namespace.declareTable(declareRequest);
+
+    // Dropping a declared-only table (no Lance dataset at the location) must still succeed
+    DropTableRequest dropRequest = new DropTableRequest();
+    dropRequest.setId(Lists.list("test_db", "declared_only_table"));
+    namespace.dropTable(dropRequest);
+
+    TableExistsRequest existsRequest = new TableExistsRequest();
+    existsRequest.setId(Lists.list("test_db", "declared_only_table"));
+    assertThrows(LanceNamespaceException.class, () -> namespace.tableExists(existsRequest));
+  }
+
+  @Test
+  public void testDeregisterTableKeepsData() throws Exception {
+    CreateNamespaceRequest nsRequest = new CreateNamespaceRequest();
+    nsRequest.setId(Lists.list("test_db"));
+    nsRequest.setMode("Create");
+    namespace.createNamespace(nsRequest);
+
+    String location = tmpDirBase + "/deregister_table";
+    DeclareTableRequest declareRequest = new DeclareTableRequest();
+    declareRequest.setId(Lists.list("test_db", "deregister_table"));
+    declareRequest.setLocation(location);
+    namespace.declareTable(declareRequest);
+
+    Schema schema =
+        new Schema(Collections.singletonList(Field.nullable("id", new ArrowType.Int(32, true))));
+    WriteParams writeParams =
+        new WriteParams.Builder().withMode(WriteParams.WriteMode.CREATE).build();
+    Dataset.create(allocator, location, schema, writeParams).close();
+
+    DeregisterTableRequest deregisterRequest = new DeregisterTableRequest();
+    deregisterRequest.setId(Lists.list("test_db", "deregister_table"));
+    namespace.deregisterTable(deregisterRequest);
+
+    TableExistsRequest existsRequest = new TableExistsRequest();
+    existsRequest.setId(Lists.list("test_db", "deregister_table"));
+    assertThrows(LanceNamespaceException.class, () -> namespace.tableExists(existsRequest));
+    assertTrue(new File(location).exists());
+  }
+
   @Test
   public void testCloseReleasesClientPool() throws Exception {
     // A freshly initialized namespace can be used and then closed (idempotently), releasing its

java/lance-namespace-hive3/src/main/java/org/lance/namespace/hive3/Hive3Namespace.java

@@ -556,6 +556,8 @@ protected void doCreateTable(
       table.setSd(sd);
 
       Map<String, String> params = Hive3Util.createLanceTableParams(properties);
+      // HMS silently rewrites the table type to MANAGED_TABLE unless this parameter is set
+      params.put("EXTERNAL", "TRUE");
       table.setParameters(params);
 
       clientPool.run(
@@ -640,12 +642,19 @@ protected String doDropTable(ObjectIdentifier id, boolean deleteData) {
       Hive3Util.validateLanceTable(hmsTable.get());
       String location = hmsTable.get().getSd().getLocation();
 
+      // HMS does not delete data for external tables, so drop metadata only and delete the
+      // Lance dataset explicitly when requested
       clientPool.run(
           client -> {
-            client.dropTable(catalog, db, tableName, deleteData, true /* ignoreUnknownTable */);
+            client.dropTable(
+                catalog, db, tableName, false /* deleteData */, true /* ignoreUnknownTable */);
             return null;
           });
 
+      if (deleteData) {
+        safeDropDataset(location);
+      }
+
       return location;
     } catch (TException | InterruptedException e) {
       if (e instanceof InterruptedException) {
@@ -656,6 +665,14 @@ protected String doDropTable(ObjectIdentifier id, boolean deleteData) {
     }
   }
 
+  private void safeDropDataset(String location) {
+    try {
+      Dataset.drop(location, Collections.emptyMap());
+    } catch (Exception e) {
+      LOG.warn("Failed to delete Lance dataset at {} for dropped table", location, e);
+    }
+  }
+
   protected Map<String, String> doDropNamespace(ObjectIdentifier id, String mode, String behavior) {
 
     try {