Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import org.apache.doris.cluster.ClusterNamespace;
import org.apache.doris.common.Config;
import org.apache.doris.common.DdlException;
import org.apache.doris.common.ErrorCode;
import org.apache.doris.common.ErrorReport;
import org.apache.doris.common.FeConstants;
import org.apache.doris.common.Pair;
import org.apache.doris.common.ThreadPoolManager;
Expand Down Expand Up @@ -1290,6 +1292,24 @@ public void dropTable(String dbName, String tableName, boolean isView, boolean i
}
throw new DdlException("Failed to get table: '" + tableName + "' in database: " + dbName);
}
// The metadata API receives no kind/lifetime flags and may purge an ordinary table.
// External catalogs have no temporary tables or MTMVs; reject mismatches before dispatch.
if (mustTemporary) {
if (ifExists) {
return;
}
ErrorReport.reportDdlException(ErrorCode.ERR_UNKNOWN_TABLE, tableName, dbName);
}
if (isMtmv) {
ErrorReport.reportDdlException(ErrorCode.ERR_WRONG_OBJECT, dbName, tableName, "MTMV",
"External catalogs do not support materialized views");
}
boolean actualIsView = dorisTable.isView();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Check HMS object kind without validating its scan format

For a nontransactional Hive table stored as Avro (also RCFile or SequenceFile), a matching DROP TABLE hms.db.t now fails here: HMSExternalTable.isView() calls makeSureInitialized(), which calls supportedHiveTable() and throws Unsupported hive input format before reaching the drop API. Previously getTableNullable() only constructed the shell, and HiveMetadataOps.dropTableImpl() checked existence/transactional status without requiring a readable format. Thus this shared guard removes a working metadata operation for tables Doris cannot scan. Resolve the kind from raw metastore metadata without scan-format validation, and cover this with a real HMS table rather than a mocked isView().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve IF EXISTS for stale HMS table names

Populate Doris' HMS table-name cache (for example with SHOW TABLES), delete t directly in Hive, then run DROP TABLE IF EXISTS hms.db.t before the name cache refreshes. getTableNullable() can still construct a cold shell from the cached name, so the null check above does not handle this case. The new HMSExternalTable.isView() call loads the now-missing table and throws HMSClientException before HiveMetadataOps.dropTableImpl() can perform its existing tableExists == false && ifExists no-op. This previously successful idempotent drop now fails. Preserve remote-missing IF EXISTS semantics during kind resolution, and test a stale name with a real HMS shell rather than only mocking a null lookup.

if (isView != actualIsView) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep failed Iceberg kind probes retryable

For an existing V1 view with a cold IcebergExternalTable shell, let the first DROP VIEW encounter a transient SQL or metadata-read error in viewExists. IcebergExternalTable.makeSureInitialized() sets objectCreated = true before assigning isView, so the failure leaves the cached shell initialized with isView = false. No drop hook invalidates it. After the backend recovers, retrying the matching DROP VIEW now hits this mismatch and reports is not VIEW until refresh or cache expiry. Before this change, dropTableImpl() probed the remote kind afresh and the retry could succeed. Publish kind initialization only after a successful probe, or obtain reliable fresh classification here; add a failure-then-retry test using the real shell.

// IF EXISTS must not relax the type check for an existing object.
ErrorReport.reportDdlException(ErrorCode.ERR_WRONG_OBJECT, dbName, tableName, isView ? "VIEW" : "TABLE",
"Use DROP " + (actualIsView ? "VIEW" : "TABLE") + " instead");
}
try {
metadataOps.dropTable(dorisTable, ifExists);
DropInfo info = new DropInfo(getName(), db.getFullName(), dorisTable.getName(), db.getFullName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
import org.apache.iceberg.expressions.Expressions;
import org.apache.iceberg.expressions.Literal;
import org.apache.iceberg.expressions.Term;
import org.apache.iceberg.jdbc.JdbcCatalog;
import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.TypeUtil;
import org.apache.iceberg.types.Types;
Expand All @@ -98,6 +99,7 @@
import java.util.TreeSet;
import java.util.concurrent.Callable;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -1954,6 +1956,19 @@ private boolean determineNestedNamespaceEnabled() {
return false;
}

private <T> T probeViews(ViewCatalog viewCatalog, Supplier<T> probe, T noViews) {
try {
return probe.get();
} catch (UnsupportedOperationException e) {
// JdbcCatalog implements ViewCatalog even for V0, which cannot store views. The SDK
// rejects these probes before SQL; do not require a V1 migration merely to read tables.
if (viewCatalog instanceof JdbcCatalog) {
return noViews;
}
throw e;
}
}

private boolean isViewCatalogEnabled() {
return viewCatalogEnabled;
}
Expand Down Expand Up @@ -2026,8 +2041,8 @@ private List<String> listTableNamesInternal(String dbName) {
}

private boolean viewExistsInternal(String remoteDbName, String remoteViewName) {
return isViewCatalogEnabled()
&& ((ViewCatalog) catalog).viewExists(getTableIdentifier(remoteDbName, remoteViewName));
return isViewCatalogEnabled() && probeViews((ViewCatalog) catalog,
() -> ((ViewCatalog) catalog).viewExists(getTableIdentifier(remoteDbName, remoteViewName)), false);
}

private View loadViewInternal(String dbName, String tblName) {
Expand All @@ -2041,8 +2056,8 @@ private List<String> listViewNamesInternal(String dbName) {
if (!isViewCatalogEnabled()) {
return Collections.emptyList();
}
return ((ViewCatalog) catalog).listViews(getNamespace(dbName))
.stream().map(TableIdentifier::name).collect(Collectors.toList());
return probeViews((ViewCatalog) catalog, () -> ((ViewCatalog) catalog).listViews(getNamespace(dbName))
.stream().map(TableIdentifier::name).collect(Collectors.toList()), Collections.emptyList());
}

private void performDropView(String remoteDbName, String remoteViewName) throws DdlException {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

package org.apache.doris.datasource;

import org.apache.doris.catalog.Env;
import org.apache.doris.common.DdlException;
import org.apache.doris.common.security.authentication.ExecutionAuthenticator;
import org.apache.doris.datasource.iceberg.IcebergMetadataOps;
import org.apache.doris.persist.EditLog;

import mockit.Mock;
import mockit.MockUp;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.jdbc.JdbcCatalog;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;

public class ExternalCatalogDropKindTest {
@Test
public void dropViewCannotDeleteV0Table() {
for (boolean ifExists : new boolean[] {false, true}) {
DropCatalog catalog = new DropCatalog(false);
Assertions.assertThrows(DdlException.class,
() -> catalog.dropTable("db", "t", true, false, ifExists, false, false));
catalog.assertTableUnchanged();
}
}

@Test
public void dropMaterializedViewCannotDeleteV0Table() {
for (boolean ifExists : new boolean[] {false, true}) {
DropCatalog catalog = new DropCatalog(false);
Assertions.assertThrows(DdlException.class,
() -> catalog.dropTable("db", "t", false, true, ifExists, false, false));
catalog.assertTableUnchanged();
}
}

@Test
public void dropTemporaryTableCannotDeleteV0Table() {
DropCatalog catalog = new DropCatalog(false);
Assertions.assertThrows(DdlException.class,
() -> catalog.dropTable("db", "t", false, false, false, true, false));
catalog.assertTableUnchanged();
}

@Test
public void dropTemporaryTableIfExistsDoesNotDeleteV0Table() throws Exception {
DropCatalog catalog = new DropCatalog(false);
catalog.dropTable("db", "t", false, false, true, true, false);
catalog.assertTableUnchanged();
}

@Test
public void dropTableCannotDeleteView() {
for (boolean ifExists : new boolean[] {false, true}) {
DropCatalog catalog = new DropCatalog(true);
Assertions.assertThrows(DdlException.class,
() -> catalog.dropTable("db", "t", false, false, ifExists, false, false));
Assertions.assertEquals(0, catalog.sdk.viewDrops);
Assertions.assertTrue(catalog.sdk.present);
Mockito.verifyNoInteractions(catalog.editLog);
}
}

@Test
public void matchingV0TableDropStillWorks() throws Exception {
DropCatalog catalog = new DropCatalog(false);
catalog.dropTable("db", "t", false, false, false, false, false);
Assertions.assertFalse(catalog.sdk.present);
Assertions.assertEquals(1, catalog.sdk.tableDrops);
Assertions.assertTrue(catalog.sdk.purge);
Mockito.verify(catalog.editLog).logDropTable(Mockito.any());
}

@Test
public void matchingViewDropStillWorks() throws Exception {
DropCatalog catalog = new DropCatalog(true);
catalog.dropTable("db", "t", true, false, false, false, false);
Assertions.assertFalse(catalog.sdk.present);
Assertions.assertEquals(1, catalog.sdk.viewDrops);
Assertions.assertEquals(0, catalog.sdk.tableDrops);
Mockito.verify(catalog.editLog).logDropTable(Mockito.any());
}

@Test
public void missingObjectIfExistsStillDoesNotMutate() throws Exception {
for (boolean view : new boolean[] {false, true}) {
DropCatalog catalog = new DropCatalog(false);
Mockito.when(catalog.database.getTableNullable("t")).thenReturn(null);
catalog.dropTable("db", "t", view, false, true, false, false);
catalog.assertTableUnchanged();
}
}

private static class DropCatalog extends ExternalCatalog {
private final RecordingJdbcCatalog sdk;
private final ExternalDatabase<ExternalTable> database;
private final EditLog editLog = Mockito.mock(EditLog.class);

private DropCatalog(boolean view) {
super(1L, "drop_kind_test", InitCatalogLog.Type.TEST, "");
initialized = true;
catalogProperty = new CatalogProperty(null, Collections.emptyMap());
executionAuthenticator = new ExecutionAuthenticator() {
};
sdk = new RecordingJdbcCatalog(view);
metadataOps = new IcebergMetadataOps(this, sdk) {
@Override
public void afterDropTable(String dbName, String tableName) {
// Cache retirement is outside this test's drop-kind and mutation boundary.
}
};
database = Mockito.mock(ExternalDatabase.class);
ExternalTable table = Mockito.mock(ExternalTable.class);
Mockito.when(database.getTableNullable("t")).thenReturn(table);
Mockito.when(database.getFullName()).thenReturn("db");
Mockito.when(table.getName()).thenReturn("t");
Mockito.when(table.getRemoteName()).thenReturn("t");
Mockito.when(table.getDbName()).thenReturn("db");
Mockito.when(table.getRemoteDbName()).thenReturn("db");
Mockito.when(table.isView()).thenReturn(view);
Env env = Mockito.mock(Env.class);
Mockito.when(env.getEditLog()).thenReturn(editLog);
new MockUp<Env>() {
@Mock
Env getCurrentEnv() {
return env;
}
};
}

private void assertTableUnchanged() {
Assertions.assertTrue(sdk.present);
Assertions.assertEquals(Arrays.asList(1, 2), sdk.rows);
Assertions.assertEquals(0, sdk.tableDrops);
Assertions.assertEquals(0, sdk.viewDrops);
Mockito.verifyNoInteractions(editLog);
}

@Override
protected void initLocalObjectsImpl() {
}

@Override
public ExternalDatabase<ExternalTable> getDbNullable(String dbName) {
return database;
}

@Override
protected List<String> listTableNamesFromRemote(SessionContext ctx, String dbName) {
return Collections.singletonList("t");
}

@Override
public boolean tableExist(SessionContext ctx, String dbName, String tblName) {
return sdk.present;
}
}

// Retain the real SDK's V0 view guard while recording the destructive table operation it used to block.
private static class RecordingJdbcCatalog extends JdbcCatalog {
private final boolean view;
private final List<Integer> rows = new ArrayList<>(Arrays.asList(1, 2));
private boolean present = true;
private boolean purge;
private int tableDrops;
private int viewDrops;

private RecordingJdbcCatalog(boolean view) {
this.view = view;
}

@Override
public boolean viewExists(TableIdentifier identifier) {
return view ? present : super.viewExists(identifier);
}

@Override
public boolean tableExists(TableIdentifier identifier) {
return present;
}

@Override
public boolean dropTable(TableIdentifier identifier, boolean purge) {
this.purge = purge;
tableDrops++;
present = false;
rows.clear();
return true;
}

@Override
public boolean dropView(TableIdentifier identifier) {
viewDrops++;
present = false;
return true;
}
}
}
Loading
Loading