Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hotfix] Fix drop database not working in terminal #3363

Merged
merged 10 commits into from
Dec 31, 2024
Merged

[hotfix] Fix drop database not working in terminal #3363

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1c0e574
Implement dropNamespace
MarigWeizhi Dec 10, 2024
85d811f
test namespace for SparkUnifiedCatalog
MarigWeizhi Dec 17, 2024
3af703c
Merge branch 'master' into spark_catalog
MarigWeizhi Dec 17, 2024
c36cc43
Merge branch 'master' into spark_catalog
huyuanfeng2018 Dec 20, 2024
6f62484
Implement cascade or non-cascading drop namespace in base
MarigWeizhi Dec 20, 2024
bb5fc51
Restore private
MarigWeizhi Dec 20, 2024
fb353d0
fix
MarigWeizhi Dec 20, 2024
fdf8179
Merge branch 'apache:master' into spark_catalog
MarigWeizhi Dec 20, 2024
4eda82f
Merge branch 'master' into spark_catalog
MarigWeizhi Dec 23, 2024
3ea9d2d
Remove redundant method
MarigWeizhi Dec 26, 2024
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
4 changes: 2 additions & 2 deletions ...ro-mixed-spark-3-common/src/main/java/org/apache/amoro/spark/SparkUnifiedCatalogBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public class SparkUnifiedCatalogBase implements TableCatalog, SupportsNamespaces
TableFormat.MIXED_ICEBERG, "org.apache.amoro.spark.MixedFormatSparkCatalog",
TableFormat.PAIMON, "org.apache.paimon.spark.SparkCatalog");

private UnifiedCatalog unifiedCatalog;
protected UnifiedCatalog unifiedCatalog;
private String name;
private final Map<TableFormat, SparkTableFormat> tableFormats = Maps.newConcurrentMap();
private final Map<TableFormat, TableCatalog> tableCatalogs = Maps.newConcurrentMap();
Expand Down Expand Up @@ -128,7 +128,7 @@ public String name() {
return name;
}

private String namespaceToDatabase(String[] namespace) {
protected String namespaceToDatabase(String[] namespace) {
Preconditions.checkArgument(namespace.length == 1, "only support namespace with 1 level.");
return namespace[0];
}
Expand Down
14 changes: 14 additions & 0 deletions ...-3-common/src/test/java/org/apache/amoro/spark/test/unified/UnifiedCatalogTestSuites.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ public void testTableFormats(TableFormat format, boolean sessionCatalog) {
long count = sql(sqlText).count();
Assertions.assertEquals(expect, count);

// create and drop namespace
testNamespaceWithSql();

// visit sub tables.
testVisitSubTable(format, sessionCatalog);

Expand All @@ -108,6 +111,17 @@ public void testTableFormats(TableFormat format, boolean sessionCatalog) {
Assertions.assertFalse(unifiedCatalog().tableExists(target().database, target().table));
}

private void testNamespaceWithSql() {
// Use SparkTestBase::sql method to test SparkUnifiedCatalog instead of CommonUnifiedCatalog.
String createDatabase = "CREATE DATABASE test_unified_catalog";
sql(createDatabase);
Assertions.assertTrue(unifiedCatalog().databaseExists("test_unified_catalog"));

String dropDatabase = "DROP DATABASE test_unified_catalog";
sql(dropDatabase);
Assertions.assertFalse(unifiedCatalog().databaseExists("test_unified_catalog"));
}

private String pkDDL(TableFormat format) {
if (TableFormat.MIXED_HIVE.equals(format) || TableFormat.MIXED_ICEBERG.equals(format)) {
return ", primary key(id)";
Expand Down
18 changes: 17 additions & 1 deletion .../v3.3/amoro-mixed-spark-3.3/src/main/java/org/apache/amoro/spark/SparkUnifiedCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.amoro.spark;

import org.apache.amoro.TableFormat;
import org.apache.amoro.TableIDWithFormat;
import org.apache.spark.sql.catalyst.analysis.NoSuchFunctionException;
import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
import org.apache.spark.sql.catalyst.analysis.NoSuchTableException;
Expand All @@ -31,6 +32,8 @@
import org.apache.spark.sql.connector.catalog.functions.UnboundFunction;
import org.apache.spark.sql.connector.iceberg.catalog.ProcedureCatalog;

import java.util.List;

public class SparkUnifiedCatalog extends SparkUnifiedCatalogBase
implements TableCatalog, SupportsNamespaces, ProcedureCatalog, FunctionCatalog {

Expand Down Expand Up @@ -86,7 +89,20 @@ public UnboundFunction loadFunction(Identifier ident) throws NoSuchFunctionExcep
@Override
public boolean dropNamespace(String[] namespace, boolean cascade)
MarigWeizhi marked this conversation as resolved.
Show resolved Hide resolved
throws NoSuchNamespaceException, NonEmptyNamespaceException {
return false;
String database = namespaceToDatabase(namespace);
if (!unifiedCatalog.databaseExists(database)) {
throw new NoSuchNamespaceException(namespace);
}
List<TableIDWithFormat> tables = unifiedCatalog.listTables(database);
if (!tables.isEmpty() && !cascade) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not drop all tables first and then drop the database?

I think should this logic be implemented directly by the parent (spark-common).
We already have concrete logic in SparkUnifiedCatalogBase.dropNamespace.
so，remove Override is enough.

WDYT?

Copy link
Contributor

@zhoujinsong zhoujinsong Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agreed that we may reuse some implementation in the parent class. However I think it is dangerous to delete all tables by default in Spark 3.2 implementation. So we may:

  • Add a method in the base class to support dropping namespace cascade or not.
  • Drop namespace by default with false for Spark 3.2

throw new NonEmptyNamespaceException(namespace);
}

for (TableIDWithFormat id : tables) {
unifiedCatalog.dropTable(database, id.getIdentifier().getTableName(), true);
}
unifiedCatalog.dropDatabase(database);
return !unifiedCatalog.databaseExists(database);
}

/**
Expand Down
Loading