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

[#3002] fix(jdbc-mysql): Fix MySQL list tables bugs. #3229

Closed
wants to merge 5 commits into from
Closed

[#3002] fix(jdbc-mysql): Fix MySQL list tables bugs. #3229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ee66ddc
Fix jdbc list table bugs.
yuqi1129 Apr 30, 2024
31f2560
Remove
yuqi1129 Apr 30, 2024
fa26d29
Add PG list table test.
yuqi1129 Apr 30, 2024
70c8fcf
Optimize test.
yuqi1129 May 6, 2024
37a4325
Merge branch 'main' of github.com:datastrato/gravitino into issue_3002
yuqi1129 May 6, 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
10 changes: 6 additions & 4 deletions ...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.datastrato.gravitino.rel.expressions.transforms.Transform;
import com.datastrato.gravitino.rel.indexes.Index;
import com.datastrato.gravitino.rel.indexes.Indexes;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
import java.sql.Connection;
import java.sql.DatabaseMetaData;
Expand Down Expand Up @@ -110,7 +111,7 @@ public void drop(String databaseName, String tableName) throws NoSuchTableExcept
public List<String> listTables(String databaseName) throws NoSuchSchemaException {
try (Connection connection = getConnection(databaseName)) {
final List<String> names = Lists.newArrayList();
try (ResultSet tables = getTables(connection)) {
try (ResultSet tables = getTables(connection, databaseName)) {
while (tables.next()) {
if (Objects.equals(tables.getString("TABLE_SCHEM"), databaseName)) {
names.add(tables.getString("TABLE_NAME"));
Expand Down Expand Up @@ -259,9 +260,9 @@ public void purge(String databaseName, String tableName) throws NoSuchTableExcep
}
}

protected ResultSet getTables(Connection connection) throws SQLException {
@VisibleForTesting
public ResultSet getTables(Connection connection, String databaseName) throws SQLException {
final DatabaseMetaData metaData = connection.getMetaData();
String databaseName = connection.getSchema();
return metaData.getTables(databaseName, databaseName, null, JdbcConnectorUtils.getTableTypes());
}

Expand Down Expand Up @@ -415,7 +416,8 @@ protected JdbcColumn getJdbcColumnFromTable(JdbcTable jdbcTable, String colName)
"Column %s does not exist in table %s", colName, jdbcTable.name()));
}

protected Connection getConnection(String catalog) throws SQLException {
@VisibleForTesting
public Connection getConnection(String catalog) throws SQLException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you verify whether the problem still exists if getConnection using setSchema(xx) not setCatalog(xx)?

    Connection getConnection(String catalog) {
       Connection connection = dataSource.getConnection();
       connection.setCatalog(catalog);
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

noticed that getTables using connection.getSchema() to retrieve tables, the total JDBC logic is muddledness ：（

  protected ResultSet getTables(Connection connection) throws SQLException {
    final DatabaseMetaData metaData = connection.getMetaData();
    String databaseName = connection.getSchema();
    return metaData.getTables(databaseName, databaseName, null, JdbcConnectorUtils.getTableTypes());
  }

Copy link
Contributor Author

@yuqi1129 yuqi1129 Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

noticed that getTables using connection.getSchema() to retrieve tables, the total JDBC logic is muddledness ：（

  protected ResultSet getTables(Connection connection) throws SQLException {
    final DatabaseMetaData metaData = connection.getMetaData();
    String databaseName = connection.getSchema();
    return metaData.getTables(databaseName, databaseName, null, JdbcConnectorUtils.getTableTypes());
  }

I have removed the line String databaseName = connection.getSchema();.

Root cause of this problem: String databaseName = connection.getSchema(); is always null.
please review it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you verify whether the problem still exists if getConnection using setSchema(xx) not setCatalog(xx)?

    Connection getConnection(String catalog) {
       Connection connection = dataSource.getConnection();
       connection.setCatalog(catalog);
    }

It's not work to use setSchema(xx);

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you verify whether the problem still exists if getConnection using setSchema(xx) not setCatalog(xx)?

    Connection getConnection(String catalog) {
       Connection connection = dataSource.getConnection();
       connection.setCatalog(catalog);
    }

It's not work to use setSchema(xx);

when you test do you remove String databaseName = connection.getSchema();

Copy link
Contributor Author

@yuqi1129 yuqi1129 May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you verify whether the problem still exists if getConnection using setSchema(xx) not setCatalog(xx)?

    Connection getConnection(String catalog) {
       Connection connection = dataSource.getConnection();
       connection.setCatalog(catalog);
    }

It's not work to use setSchema(xx);

when you test do you remove String databaseName = connection.getSchema();

No, the method:

  protected ResultSet getTables(Connection connection) throws SQLException {
    final DatabaseMetaData metaData = connection.getMetaData();
    String databaseName = connection.getSchema();

    // We need the database name here.
    return metaData.getTables(databaseName, databaseName, null, JdbcConnectorUtils.getTableTypes());
  }

The value of the schema name must be passed as a method parameter, otherwise we cannot remove it.

Connection connection = dataSource.getConnection();
connection.setCatalog(catalog);
return connection;
Expand Down
2 changes: 1 addition & 1 deletion .../src/test/java/com/datastrato/gravitino/catalog/jdbc/operation/SqliteTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ protected boolean getAutoIncrementInfo(ResultSet columns) {
public List<String> listTables(String databaseName) throws NoSuchSchemaException {
try (Connection connection = getConnection(databaseName)) {
final List<String> names = Lists.newArrayList();
try (ResultSet tables = getTables(connection)) {
try (ResultSet tables = getTables(connection, databaseName)) {
// tables.getString("TABLE_SCHEM") is always null.
while (tables.next()) {
names.add(tables.getString("TABLE_NAME"));
Expand Down
2 changes: 1 addition & 1 deletion .../src/main/java/com/datastrato/gravitino/catalog/doris/operation/DorisTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public List<String> listTables(String databaseName) throws NoSuchSchemaException
final List<String> names = Lists.newArrayList();

try (Connection connection = getConnection(databaseName);
ResultSet tables = getTables(connection)) {
ResultSet tables = getTables(connection, databaseName)) {
while (tables.next()) {
if (Objects.equals(tables.getString("TABLE_CAT"), databaseName)) {
names.add(tables.getString("TABLE_NAME"));
Expand Down
13 changes: 10 additions & 3 deletions .../src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,20 @@ public List<String> listTables(String databaseName) throws NoSuchSchemaException
final List<String> names = Lists.newArrayList();

try (Connection connection = getConnection(databaseName);
ResultSet tables = getTables(connection)) {
ResultSet tables = getTables(connection, databaseName)) {
long total = 0;
while (tables.next()) {
if (Objects.equals(tables.getString("TABLE_CAT"), databaseName)) {
String dbNameFromResult = tables.getString("TABLE_CAT");
if (Objects.equals(dbNameFromResult, databaseName)) {
names.add(tables.getString("TABLE_NAME"));
}
total++;
}
LOG.info("Finished listing tables size {} for database name {} ", names.size(), databaseName);
LOG.info(
"Finished listing tables size {} for database name {}, total scan = {}",
names.size(),
databaseName,
total);
return names;
} catch (final SQLException se) {
throw this.exceptionMapper.toGravitinoException(se);
Expand Down
44 changes: 44 additions & 0 deletions .../java/com/datastrato/gravitino/catalog/mysql/integration/test/MysqlTableOperationsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
import com.datastrato.gravitino.rel.types.Type;
import com.datastrato.gravitino.rel.types.Types;
import com.datastrato.gravitino.utils.RandomNameUtils;
import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -771,6 +774,47 @@ public void testCreateMultipleTables() {
Assertions.assertFalse(tables.contains(test_table_2));
}

@Test
void testListMySQLTable() throws SQLException {
String[] dbNames = new String[] {"db1", "db2"};
String[] tableNames = new String[] {"table1", "table2", "table3"};
for (String dbName : dbNames) {
DATABASE_OPERATIONS.create(dbName, null, null);
}

for (String dbName : dbNames) {
for (String tableName : tableNames) {
TABLE_OPERATIONS.create(
dbName,
tableName,
new JdbcColumn[] {
JdbcColumn.builder()
.withName("col_1")
.withType(Types.DecimalType.of(10, 2))
.withComment("test_decimal")
.withNullable(false)
.build()
},
"test_comment",
null,
null,
Distributions.NONE,
Indexes.EMPTY_INDEXES);
}
}

for (String dbName : dbNames) {
try (Connection connection = TABLE_OPERATIONS.getConnection(dbName);
ResultSet resultSet = TABLE_OPERATIONS.getTables(connection, dbName)) {
List<String> tables = new ArrayList<>();
while (resultSet.next()) {
tables.add(resultSet.getString("TABLE_NAME"));
}
Assertions.assertEquals(Arrays.asList(tableNames), tables);
}
}
}

@Test
public void testLoadTableDefaultProperties() {
String test_table_1 = RandomNameUtils.genRandomName("properties_table_");
Expand Down
2 changes: 1 addition & 1 deletion ...java/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ protected ResultSet getPrimaryKeys(String schemaName, String tableName, Database
}

@Override
protected Connection getConnection(String schema) throws SQLException {
public Connection getConnection(String schema) throws SQLException {
Connection connection = dataSource.getConnection();
connection.setCatalog(database);
connection.setSchema(schema);
Expand Down
43 changes: 43 additions & 0 deletions ...datastrato/gravitino/catalog/postgresql/integration/test/PostgreSqlTableOperationsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@
import com.datastrato.gravitino.rel.types.Types;
import com.datastrato.gravitino.utils.RandomNameUtils;
import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -622,4 +624,45 @@ public void testCreateIndexTable() {
gravitinoRuntimeException.getMessage(),
"column \"no_exist_1\" named in key does not exist"));
}

@Test
void testListPostgreSQLTable() throws SQLException {
String[] dbNames = new String[] {"db1", "db2"};
String[] tableNames = new String[] {"table1", "table2", "table3"};
for (String dbName : dbNames) {
DATABASE_OPERATIONS.create(dbName, null, null);
}

for (String dbName : dbNames) {
for (String tableName : tableNames) {
TABLE_OPERATIONS.create(
dbName,
tableName,
new JdbcColumn[] {
JdbcColumn.builder()
.withName("col_1")
.withType(Types.DecimalType.of(10, 2))
.withComment("test_decimal")
.withNullable(false)
.build()
},
"test_comment",
null,
null,
Distributions.NONE,
Indexes.EMPTY_INDEXES);
}
}

for (String dbName : dbNames) {
try (Connection connection = TABLE_OPERATIONS.getConnection(dbName);
ResultSet resultSet = TABLE_OPERATIONS.getTables(connection, dbName)) {
List<String> tables = new ArrayList<>();
while (resultSet.next()) {
tables.add(resultSet.getString("TABLE_NAME"));
}
Assertions.assertEquals(Arrays.asList(tableNames), tables);
}
}
}
}
Loading