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

[FLINK-34088] [hotfix] fix the problems with special table name characters of postgres and oracle  and sqlserver. #89

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BlackPigHe
Copy link

[hotfix] fix the problems with special table name characters of postgres and oracle  and sqlserver.

Copy link

boring-cyborg bot commented Jan 16, 2024

Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)

@BlackPigHe BlackPigHe changed the title [hotfix] fix the problems with special table name characters of postgres and oracle  and sqlserver. [FLINK-34088] [hotfix] fix the problems with special table name characters of postgres and oracle  and sqlserver. Jan 16, 2024
…res and oracle and sqlserver. update test case.
@BlackPigHe
Copy link
Author

add the commit to update test case

hezijiang@deepexi.com added 2 commits January 17, 2024 15:39
…in oracle jdbc,Specify the required type explicitly
…roperly in oracle jdbc,Specify the required type explicitly"

This reverts commit 7272a79.
Comment on lines +172 to +180
public static String handleDoubleQuotes(String identifier) {
String[] split = identifier.split("\\.");
StringBuilder builder = new StringBuilder();
for (String s : split) {
builder.append("\"").append(s).append("\"");
builder.append(".");
}
return builder.deleteCharAt(builder.length() - 1).toString();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't get why we are going to put logic with double quotes into some generic place if for different engines there could be different quotes?

Copy link
Author

Choose a reason for hiding this comment

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

Because oracle and postgres have the same escape characters, It's all double quotes

Copy link
Contributor

Choose a reason for hiding this comment

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

this package is not only for oracle and postgres. E.g. MS SQL Server, MySQL are different

Copy link
Author

Choose a reason for hiding this comment

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

Do you think that the location of this method is not appropriate, I think it is OK, the specific escape characters of the data source in their own package implementation, there are public features to extract and did not find a better location

Comment on lines +51 to +54
"INSERT INTO \"tbl\"(\"id\", \"name\", \"email\", \"ts\", \"field1\", \"field_2\", \"__field_3__\") "
+ "VALUES (:id, :name, :email, :ts, :field1, :field_2, :__field_3__)");
NamedStatementMatcher.parsedSql(
"INSERT INTO tbl(id, name, email, ts, field1, field_2, __field_3__) "
"INSERT INTO \"tbl\"(\"id\", \"name\", \"email\", \"ts\", \"field1\", \"field_2\", \"__field_3__\") "
Copy link
Contributor

Choose a reason for hiding this comment

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

In issue description it was only something about table name
why does it impact columns?

Copy link
Author

Choose a reason for hiding this comment

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

the method ‘quoteIdentifier’ is common,column metadata is also by call the method.Columns should have this problem as well

Copy link
Contributor

@snuyanzin snuyanzin Jan 19, 2024

Choose a reason for hiding this comment

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

can we have a failing test reproducing the problem?

@snuyanzin
Copy link
Contributor

I guess we can extract information about current quote identifier directly from JDBCDriver
java.sql.DatabaseMetaData#getIdentifierQuoteString
and then we don't need to hardcode it for every dialect

@BlackPigHe
Copy link
Author

I guess we can extract information about current quote identifier directly from JDBCDriver java.sql.DatabaseMetaData#getIdentifierQuoteString and then we don't need to hardcode it for every dialect

I get it. If there are no compatibility issues, It's better this way.

@BlackPigHe
Copy link
Author

quote identifier

I thought the method Java.SQL.DatabaseMetaData#getIdentifierQuoteString to dynamically access quote identifier, the price is too big, need to introduce Connection object which is very heavy, It is not reasonable for the existing dialect class design.

@snuyanzin
Copy link
Contributor

yep, may be it is a point to see whether we could adapt it or not

btw I haven't found any test reproducing the issue, can you add one?
Also I would say the issue is more complex than just adding sql quote identifiers
e.g. even though there are quotes queries with fields containing question marks ? will fail FLINK-34211
i guess we need to a have a number of dedicated tests for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants