[#2115] fix(jdbc-catalog): Can't create a table in database with the same name prefix. #2116
Conversation
yuqi1129
added
branch-0.4
need backport
| @@ -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 { | |||
There was a problem hiding this comment.
Why haven't we implemented this logic in the superclass
There was a problem hiding this comment.
Because PG and MySQL are different here, only PG has the problem mentioned.
There was a problem hiding this comment.
could we try to fix it in JdbcTableOperations#load()? it's more general. It's hard to maintaince to override load method.
There was a problem hiding this comment.
@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.
|
@Clearvive |
|
Should you continue this work @yuqi1129 ? |
Yeah, it's currently under review. |
|
@FANNG1 |
yuqi1129
changed the title
yuqi1129
added
need backport
|
@FANNG1 |
| * @param resultSet The result set of the table | ||
| * @return The builder of the table to be returned | ||
| */ | ||
| protected JdbcTable.Builder attachTableToBuilder( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@mchades
Please also take a look about it, thanks.
|
MySLQ is OK with a wildcard in the 
|
| protected JdbcTable.Builder getTableBuilder( | ||
| ResultSet resultSet, String databaseName, String tableName) throws SQLException { | ||
| boolean found = false; | ||
| // Handle case-sensitive issues. |
There was a problem hiding this comment.
What does this comment refer to? I don't see any code below that handles case sensitivity.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's not part of this PR, it was added previously.
There was a problem hiding this comment.
The comment should be clearer.
There was a problem hiding this comment.
The comment should be clearer.
added.
…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`
…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`
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