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.

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

[#2115] fix(jdbc-catalog): Can't create a table in database with the same name prefix. #2116

Merged
merged 13 commits into from
Apr 23, 2024

Conversation

yuqi1129
Copy link
Contributor

@yuqi1129 yuqi1129 commented Feb 6, 2024

What changes were proposed in this pull request?

Add check logic about schema name when loading table meta from driver.

Why are the changes needed?

Some drivers , such as PG drivers, contain schema name information, we need to filter it. Some drivers like MySQL don't like it, we do not need to check it.
check it.

Fix: #2115

Does this PR introduce any user-facing change?

N/A.

How was this patch tested?

Add ITs: testCreateSameTableInDifferentSchema

@yuqi1129 yuqi1129 added branch-0.4 need backport Issues that need to backport to another branch and removed need backport Issues that need to backport to another branch branch-0.4 labels Feb 6, 2024
@@ -58,6 +60,62 @@ public void initialize(
"The `jdbc-database` configuration item is mandatory in PostgreSQL.");
}

@Override
public JdbcTable load(String databaseName, String tableName) throws NoSuchTableException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why haven't we implemented this logic in the superclass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because PG and MySQL are different here, only PG has the problem mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

could we try to fix it in JdbcTableOperations#load()? it's more general. It's hard to maintaince to override load method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FANNG1
For PG and MySQL, the following logic is different and I believe the logic vary according to different JDBC implementations.

      while (table.next() && !found) {
        String tableNameInResult = table.getString("TABLE_NAME");

        // PG and MySQL are different in the column name `TABLE_SCHEM`
        String tableSchemaInResultLowerCase = table.getString("TABLE_SCHEM");
        if (Objects.equals(tableNameInResult, tableName)
            && Objects.equals(tableSchemaInResultLowerCase, databaseName)) {
          jdbcTableBuilder = getBasicJdbcTableInfo(table);
          found = true;
        }
      }

So I prefer to include this detailed implementation in the corresponding implementations.

@yuqi1129
Copy link
Contributor Author

@Clearvive
Can you please help review it or raise a PR about these related issues here? Please follow these issues and other related ones.

@yuqi1129 yuqi1129 self-assigned this Feb 23, 2024
@jerryshao
Copy link
Contributor

Should you continue this work @yuqi1129 ?

@yuqi1129
Copy link
Contributor Author

Should you continue this work @yuqi1129 ?

Yeah, it's currently under review.

@yuqi1129
Copy link
Contributor Author

@diqiu50 @FANNG1
Could you kindly review it for me?

@yuqi1129 yuqi1129 added the upload log Always upload container log label Apr 11, 2024
@yuqi1129 yuqi1129 closed this Apr 11, 2024
@yuqi1129 yuqi1129 reopened this Apr 11, 2024
@yuqi1129
Copy link
Contributor Author

@FANNG1
Please help me review this PR if you are free.

@yuqi1129 yuqi1129 changed the title [#2115] fix(jdbc-catalog): Can't create a table that exist in other database. [#2115] fix(jdbc-catalog): Can't create a table in database with the same name prefix. Apr 18, 2024
@yuqi1129 yuqi1129 added need backport Issues that need to backport to another branch branch-0.5 and removed upload log Always upload container log labels Apr 23, 2024
@yuqi1129
Copy link
Contributor Author

@FANNG1
Code updated, please take a look again, thanks.

* @param resultSet The result set of the table
* @return The builder of the table to be returned
*/
protected JdbcTable.Builder attachTableToBuilder(
Copy link
Contributor

Choose a reason for hiding this comment

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

seems the difference is whether check schema , how about add an abstract method like boolean supportsSchema()? if yes, check the schema, if not, do normal logic.

Copy link
Contributor Author

@yuqi1129 yuqi1129 Apr 23, 2024

Choose a reason for hiding this comment

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

supportsSchema() does not seem to be a very desirable name, we may name it needToCheckSchemaName or something similar.

I don't think the syntax supportSchema can be explained clearly in this sentence. I have confirmed that there are no similar issues with MySQL. It's all about the behavior of JDBC metadata provided by the driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mchades
Please also take a look about it, thanks.

@yuqi1129
Copy link
Contributor Author

MySLQ is OK with a wildcard in the db name. assuming there are three database name db10, db1_, db12, and each database contains a table name t, then when I query the table t in database db1_, I only get 1 result not 3.

image

diqiu50
diqiu50 previously approved these changes Apr 23, 2024
Copy link
Contributor

@diqiu50 diqiu50 left a comment

Choose a reason for hiding this comment

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

LGTM

protected JdbcTable.Builder getTableBuilder(
ResultSet resultSet, String databaseName, String tableName) throws SQLException {
boolean found = false;
// Handle case-sensitive issues.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment refer to? I don't see any code below that handles case sensitivity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the code Objects.equals(resultSet.getString("TABLE_NAME"), tableName), Previously, we would get T or t if we use keyword t to query table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not part of this PR, it was added previously.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment should be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment should be clearer.

added.

Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM

@mchades mchades merged commit 76376b0 into apache:main Apr 23, 2024
22 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 23, 2024
…same name prefix. (#2116)

### What changes were proposed in this pull request?

Add check logic about schema name when loading table meta from driver. 

### Why are the changes needed?

Some drivers , such as PG drivers, contain schema name information, we
need to filter it. Some drivers like MySQL don't like it, we do not need
to check it.
check it. 

Fix: #2115 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

Add ITs: `testCreateSameTableInDifferentSchema`
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jun 13, 2024
…h the same name prefix. (apache#2116)

### What changes were proposed in this pull request?

Add check logic about schema name when loading table meta from driver. 

### Why are the changes needed?

Some drivers , such as PG drivers, contain schema name information, we
need to filter it. Some drivers like MySQL don't like it, we do not need
to check it.
check it. 

Fix: apache#2115 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

Add ITs: `testCreateSameTableInDifferentSchema`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-0.5 need backport Issues that need to backport to another branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug report] Can't create a table that exists in other schema
5 participants