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

[core]Fix drop table in Hive and jdbc catalog when the file path does not exists. #4860

Closed
wants to merge 2 commits into from
+100 −6
Closed

[core]Fix drop table in Hive and jdbc catalog when the file path does not exists. #4860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a271c4c
fix drop table in HiveCatalog and jdbcCatalog when file path is not e…
herefree Jan 8, 2025
8704d34
fix
herefree Jan 8, 2025
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
13 changes: 7 additions & 6 deletions paimon-core/src/main/java/org/apache/paimon/catalog/AbstractCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,16 +268,13 @@ public void dropTable(Identifier identifier, boolean ignoreIfNotExists)
throws TableNotExistException {
checkNotBranch(identifier, "dropTable");
checkNotSystemTable(identifier, "dropTable");

try {
getTable(identifier);
} catch (TableNotExistException e) {
if (!tableExists(identifier)) {
if (ignoreIfNotExists) {
return;
} else {
throw new TableNotExistException(identifier);
}
throw new TableNotExistException(identifier);
}

dropTableImpl(identifier);
}

Expand Down Expand Up @@ -598,6 +595,10 @@ public Optional<TableSchema> tableSchemaInFileSystem(Path tablePath, String bran
});
}

protected boolean tableExists(Identifier identifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

private

In hive and jdbc catalog, tableExists method has different implement. So I used protected.

return tableExistsInFileSystem(getTableLocation(identifier), DEFAULT_MAIN_BRANCH);
}

/** Table metadata. */
protected static class TableMeta {

Expand Down
5 changes: 5 additions & 0 deletions paimon-core/src/main/java/org/apache/paimon/jdbc/JdbcCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,11 @@ protected TableSchema getDataTableSchema(Identifier identifier) throws TableNotE
() -> new RuntimeException("There is no paimon table in " + tableLocation));
}

protected boolean tableExists(Identifier identifier) {
return JdbcUtils.tableExists(
connections, catalogKey, identifier.getDatabaseName(), identifier.getTableName());
}

@Override
public boolean caseSensitive() {
return false;
Expand Down
29 changes: 29 additions & 0 deletions paimon-core/src/test/java/org/apache/paimon/jdbc/JdbcCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.apache.paimon.catalog.Catalog;
import org.apache.paimon.catalog.CatalogTestBase;
import org.apache.paimon.catalog.Identifier;
import org.apache.paimon.fs.Path;
import org.apache.paimon.options.CatalogOptions;
import org.apache.paimon.options.Options;
import org.apache.paimon.table.Table;
Expand All @@ -33,6 +34,7 @@
import java.io.ByteArrayOutputStream;
import java.io.ObjectOutputStream;
import java.sql.SQLException;
import java.util.List;
import java.util.Map;
import java.util.UUID;

Expand Down Expand Up @@ -117,6 +119,33 @@ public void testSerializeTable() throws Exception {
});
}

@Test
public void testDropTable() throws Exception {
String databaseName = "test_db";
String tableName = "new_table";
catalog.createDatabase(databaseName, false);
catalog.createTable(
Identifier.create(databaseName, tableName), DEFAULT_TABLE_SCHEMA, false);
JdbcCatalog jdbcCatalog = (JdbcCatalog) catalog;
Path path = jdbcCatalog.getTableLocation(Identifier.create(databaseName, tableName));
// delete file path
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "delete table path" is better?

catalog.fileIO().deleteDirectoryQuietly(path);

assertThatThrownBy(() -> catalog.getTable(Identifier.create(databaseName, tableName)))
.isInstanceOf(RuntimeException.class)
.hasMessage(
"There is no paimon table in "
+ jdbcCatalog.getTableLocation(
Identifier.create(databaseName, tableName)));

List<String> tablesCon = catalog.listTables(databaseName);
assertThat(tablesCon).contains(tableName);

catalog.dropTable(Identifier.create(databaseName, tableName), false);
List<String> tables = catalog.listTables(databaseName);
assertThat(tableName).isNotIn(tables);
}

@Test
public void testAlterDatabase() throws Exception {
this.alterDatabaseWhenSupportAlter();
Expand Down
16 changes: 16 additions & 0 deletions paimon-hive/paimon-hive-catalog/src/main/java/org/apache/paimon/hive/HiveCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1442,6 +1442,22 @@ public static String possibleHiveConfPath() {
return System.getenv("HIVE_CONF_DIR");
}

protected boolean tableExists(Identifier identifier) {
try {
return clients.run(
client ->
client.tableExists(
identifier.getDatabaseName(), identifier.getTableName()));
} catch (TException e) {
throw new RuntimeException(
"Cannot determine if table " + identifier.getFullName() + "exists.", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

"exists." -> " exists."

} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException(
"Interrupted in call to tableExists " + identifier.getFullName(), e);
}
}

public int getBatchGetTableSize() {
try {
int size =
Expand Down
43 changes: 43 additions & 0 deletions paimon-hive/paimon-hive-catalog/src/test/java/org/apache/paimon/hive/HiveCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.paimon.catalog.CatalogTestBase;
import org.apache.paimon.catalog.Identifier;
import org.apache.paimon.client.ClientPool;
import org.apache.paimon.fs.Path;
import org.apache.paimon.options.CatalogOptions;
import org.apache.paimon.options.Options;
import org.apache.paimon.schema.Schema;
Expand Down Expand Up @@ -60,6 +61,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.fail;

/** Tests for {@link HiveCatalog}. */
Expand Down Expand Up @@ -277,6 +279,47 @@ public void testAlterHiveTableParameters() {
}
}

@Test
public void testDropTable() {
try {
String databaseName = "test_db";
catalog.createDatabase(databaseName, false);
String tableName = "new_table";
Map<String, String> options = new HashMap<>();
Schema addHiveTableParametersSchema =
new Schema(
Lists.newArrayList(
new DataField(0, "pk", DataTypes.INT()),
new DataField(1, "col1", DataTypes.STRING()),
new DataField(2, "col2", DataTypes.STRING())),
Collections.emptyList(),
Collections.emptyList(),
options,
"");

catalog.createTable(
Identifier.create(databaseName, tableName),
addHiveTableParametersSchema,
false);
// delete file path
Copy link
Contributor

Choose a reason for hiding this comment

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

dito

HiveCatalog hiveCatalog = (HiveCatalog) catalog;
Path path = hiveCatalog.getTableLocation(Identifier.create(databaseName, tableName));
catalog.fileIO().deleteDirectoryQuietly(path);
assertThrows(
Catalog.TableNotExistException.class,
() -> catalog.getTable(Identifier.create(databaseName, tableName)));

List<String> tablesCon = catalog.listTables(databaseName);
assertThat(tablesCon).contains(tableName);

catalog.dropTable(Identifier.create(databaseName, tableName), false);
List<String> tables = catalog.listTables(databaseName);
assertThat(tableName).isNotIn(tables);
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This try statement is very big, maybe add "throw Exception" in the method signature is better?

fail("Test failed due to exception: " + e.getMessage());
}
}

@Test
public void testListTablesLock() {
try {
Expand Down
Loading