HIVE-29308: Exception when JDBC table names are case-sensitive - #6727
HIVE-29308: Exception when JDBC table names are case-sensitive#6727soumyakanti3578 wants to merge 2 commits into
Conversation
thomasrebele
left a comment
thomasrebele
left a comment
There was a problem hiding this comment.
Thanks for the PR! The approach LGTM. I've left some comments. There are a few Sonar warnings as well.
| public void testQuotedTableWithSchemaOnlyEncodesTable() throws Exception { | ||
| // The historical authorization URI only contains the table name; the schema does not affect it. | ||
| URI uri = authUri("jdbc:postgresql://host:5432/db", "\"World\"", "\"Country\""); | ||
| assertEquals("jdbc:postgresql://host:5432/db/Country", uri.toString()); |
There was a problem hiding this comment.
Shouldn't the JDBC URL contain the schema? I've found a Stackoverflow question. There is a param currentSchema for Postgres JDBC, and search_path (or searchpath?) for non-JDBC URLs.
Changing the URL for this case might be out-of-scope for HIVE-29308.
There was a problem hiding this comment.
Yeah this seems out of scope for this PR, but this can be done in a follow up.
| return identifier; | ||
| } | ||
| String inner = identifier.substring(1, identifier.length() - 1); | ||
| // A literal quote char inside a quoted identifier is escaped by doubling it. |
There was a problem hiding this comment.
This also works for SQL Server, mysql, and mariadb. How about A use of the closing char inside the table name is escaped by doubling it.. That makes it clearer that the comment is not referring to the quote char '.
There was a problem hiding this comment.
I see what you mean, and I will update it! Thanks
|
|
||
| EXPLAIN CBO SELECT COUNT(*) FROM country_mixed; | ||
| SELECT COUNT(*) FROM country_mixed; | ||
| SELECT * FROM country_mixed ORDER BY id; |
There was a problem hiding this comment.
ql/src/test/queries/clientpositive/jdbc_case_sensitive_table_mssql.q doesn't execute SELECT * FROM country_mixed ORDER BY id;. Maybe remove that query from all q files to align them?
There was a problem hiding this comment.
Yes that query is not strictly required so I will remove it from others as well.
| private static String encodeIdentifierForAuth(String identifier) { | ||
| String physical = unquoteJdbcIdentifier(identifier); | ||
| if (physical == null) { | ||
| return null; |
There was a problem hiding this comment.
Why not throwing an exception here (or in unquoteJdbcIdentifier)? If we return null the JDBC URL will become hostUrl + "/null", which does not seem to be a clean way to specify a table with id "null".
There was a problem hiding this comment.
I will throw an exception in encodeIdentifierForAuth as unquoteJdbcIdentifier is more general purpose.
| String table_name = tableProperties.get(Constants.JDBC_TABLE); | ||
| return new URI(host_url+"/"+table_name); | ||
| // Encode only the auth-resource path segment to keep URI construction valid; this does not | ||
| // alter the JDBC URL used by the driver for actual query execution. |
There was a problem hiding this comment.
It's not clear to me what "auth-resource path segment" is referring to. Also, it would be nice to give a hint where this URI is used.
Could we say Encode only the table name to keep URI construction valid; the URI is not used in the JDBC URL, but as an identifier stored in the HMS? Not sure whether that's the actual usage, I've tried to infer the meaning from the callers of the method.
There was a problem hiding this comment.
I will update it 👍🏼
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
tests unstable
labels
|
3 participants
What changes were proposed in this pull request?
Support case-sensitive tables in jdbc
Why are the changes needed?
https://issues.apache.org/jira/browse/HIVE-29308
Does this PR introduce any user-facing change?
No, except now tables can be case sensitive
How was this patch tested?