-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-29308: Exception when JDBC table names are case-sensitive #6727
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| -- country and `Country` are distinct tables. | ||
| CREATE SCHEMA bob; | ||
|
|
||
| CREATE TABLE bob.country | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into bob.country values (1, 'India'); | ||
| insert into bob.country values (2, 'Russia'); | ||
| insert into bob.country values (3, 'USA'); | ||
|
|
||
| CREATE TABLE bob.`Country` | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into bob.`Country` values (10, 'Italy'); | ||
| insert into bob.`Country` values (11, 'Greece'); | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| -- A case-sensitive collation makes country and [Country] distinct tables. | ||
| CREATE DATABASE worldcs COLLATE Latin1_General_CS_AS; | ||
| USE worldcs; | ||
|
|
||
| CREATE SCHEMA bob; | ||
|
|
||
| CREATE TABLE bob.country | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into bob.country values (1, 'India'); | ||
| insert into bob.country values (2, 'Russia'); | ||
| insert into bob.country values (3, 'USA'); | ||
|
|
||
| CREATE TABLE bob.[Country] | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into bob.[Country] values (10, 'Italy'); | ||
| insert into bob.[Country] values (11, 'Greece'); | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| -- country and `Country` are distinct tables. | ||
| CREATE TABLE country | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into country values (1, 'India'); | ||
| insert into country values (2, 'Russia'); | ||
| insert into country values (3, 'USA'); | ||
|
|
||
| CREATE TABLE `Country` | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into `Country` values (10, 'Italy'); | ||
| insert into `Country` values (11, 'Greece'); | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| -- country (unquoted, folded to COUNTRY) and "Country" (quoted, case-sensitive) are distinct tables. | ||
| ALTER SESSION SET CONTAINER = XEPDB1; | ||
|
|
||
| CREATE USER bob IDENTIFIED BY bobpass; | ||
| ALTER USER bob QUOTA UNLIMITED ON users; | ||
| GRANT CREATE SESSION TO bob; | ||
|
|
||
| CREATE TABLE bob.country | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into bob.country values (1, 'India'); | ||
| insert into bob.country values (2, 'Russia'); | ||
| insert into bob.country values (3, 'USA'); | ||
|
|
||
| CREATE TABLE bob."Country" | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into bob."Country" values (10, 'Italy'); | ||
| insert into bob."Country" values (11, 'Greece'); | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| -- country (unquoted, lowercase) and "Country" (quoted, case-sensitive) are distinct tables. | ||
| CREATE SCHEMA bob; | ||
|
|
||
| CREATE TABLE bob.country | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into bob.country values (1, 'India'); | ||
| insert into bob.country values (2, 'Russia'); | ||
| insert into bob.country values (3, 'USA'); | ||
|
|
||
| CREATE TABLE bob."Country" | ||
| ( | ||
| id int, | ||
| name varchar(20) | ||
| ); | ||
| insert into bob."Country" values (10, 'Italy'); | ||
| insert into bob."Country" values (11, 'Greece'); | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,13 +41,17 @@ | |
|
|
||
| import java.io.IOException; | ||
| import java.lang.IllegalArgumentException; | ||
| import java.net.URLEncoder; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Properties; | ||
| import java.net.URI; | ||
| import java.net.URISyntaxException; | ||
|
|
||
| import static org.apache.hadoop.hive.ql.exec.Utilities.unquoteJdbcIdentifier; | ||
|
|
||
| public class JdbcStorageHandler implements HiveStorageHandler { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(JdbcStorageHandler.class); | ||
|
|
@@ -107,10 +111,20 @@ | |
| Map<String, String> tableProperties = HiveCustomStorageHandlerUtils.getTableProperties(table); | ||
| DatabaseType dbType = DatabaseType.valueOf( | ||
| tableProperties.get(JdbcStorageConfig.DATABASE_TYPE.getPropertyName())); | ||
| String host_url = DatabaseType.METASTORE == dbType ? | ||
| String hostUrl = DatabaseType.METASTORE == dbType ? | ||
| "jdbc:metastore://" : tableProperties.get(Constants.JDBC_URL); | ||
| 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. | ||
| String tableName = encodeIdentifierForAuth(tableProperties.get(Constants.JDBC_TABLE)); | ||
| return new URI(hostUrl + "/" + tableName); | ||
|
Check warning on line 119 in jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcStorageHandler.java
|
||
| } | ||
|
|
||
| private static String encodeIdentifierForAuth(String identifier) { | ||
| String physical = unquoteJdbcIdentifier(identifier); | ||
| if (physical == null) { | ||
| return null; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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". |
||
| } | ||
| return URLEncoder.encode(physical, StandardCharsets.UTF_8); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| /* | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
|
Check warning on line 2 in jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/TestJdbcStorageHandlerAuthUri.java
|
||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.hive.storage.jdbc; | ||
|
|
||
| import org.apache.hadoop.hive.metastore.api.SerDeInfo; | ||
| import org.apache.hadoop.hive.metastore.api.StorageDescriptor; | ||
| import org.apache.hadoop.hive.metastore.api.Table; | ||
| import org.junit.Test; | ||
|
|
||
| import java.net.URI; | ||
| import java.net.URISyntaxException; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| import static org.junit.Assert.assertEquals; | ||
|
|
||
| /** | ||
|
Check warning on line 28 in jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/TestJdbcStorageHandlerAuthUri.java
|
||
| * Unit tests for {@link JdbcStorageHandler#getURIForAuth(Table)} | ||
| */ | ||
| public class TestJdbcStorageHandlerAuthUri { | ||
|
|
||
| private static Table tableWith(Map<String, String> params) { | ||
| Table table = new Table(); | ||
| table.setParameters(params); | ||
| StorageDescriptor sd = new StorageDescriptor(); | ||
| sd.setSerdeInfo(new SerDeInfo("serde", "serde.lib", new HashMap<>())); | ||
| table.setSd(sd); | ||
| return table; | ||
| } | ||
|
|
||
| private URI authUri(String jdbcUrl, String schema, String table) throws URISyntaxException { | ||
| Map<String, String> params = new HashMap<>(); | ||
| params.put("hive.sql.database.type", "POSTGRES"); | ||
| params.put("hive.sql.jdbc.url", jdbcUrl); | ||
| if (schema != null) { | ||
| params.put("hive.sql.schema", schema); | ||
| } | ||
| if (table != null) { | ||
| params.put("hive.sql.table", table); | ||
| } | ||
| return new JdbcStorageHandler().getURIForAuth(tableWith(params)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUnquotedTableUnchanged() throws Exception { | ||
| URI uri = authUri("jdbc:postgresql://host:5432/db", null, "country"); | ||
| assertEquals("jdbc:postgresql://host:5432/db/country", uri.toString()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testQuotedTableStripsQuotesPreservingCase() throws Exception { | ||
| URI uri = authUri("jdbc:postgresql://host:5432/db", null, "\"Country\""); | ||
| // The physical identifier keeps its original case, and the surrounding quotes are stripped. | ||
| assertEquals("jdbc:postgresql://host:5432/db/Country", uri.toString()); | ||
| } | ||
|
|
||
| @Test | ||
| 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()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the JDBC URL contain the schema? I've found a Stackoverflow question. There is a param Changing the URL for this case might be out-of-scope for HIVE-29308. |
||
| } | ||
|
|
||
| @Test | ||
| public void testTableWithSpecialCharactersIsUriSafe() throws Exception { | ||
| // A quoted identifier may legally contain characters that are illegal in a raw URI; they must be encoded. | ||
| URI uri = authUri("jdbc:postgresql://host:5432/db", null, "\"Odd/Name\""); | ||
| // '/' -> %2F ; the resulting string is a valid URI. | ||
| assertEquals("jdbc:postgresql://host:5432/db/Odd%2FName", uri.toString()); | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5151,4 +5151,34 @@ public static void setTableCreateTime(Configuration conf, Table table) { | |
| public static int getTableCreateTime(Configuration conf, String tableName) { | ||
| return conf.getInt(String.format("%s.%s", tableName, CREATE_TIME), 0); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the physical (unquoted) form of a JDBC identifier supplied through table properties such as | ||
| * {@code hive.sql.table} or {@code hive.sql.schema}, for use in contexts that need the identifier exactly as | ||
| * stored in the remote catalog (JDBC metadata lookups, Calcite resolution, authorization URIs). | ||
| * | ||
| * <p>Recognises ANSI/Oracle/Postgres double quotes ({@code "id"}), MySQL/MariaDB back-ticks ({@code `id`}) and | ||
| * SQL Server brackets ({@code [id]}). Unquoted identifiers are returned unchanged. | ||
| */ | ||
| public static String unquoteJdbcIdentifier(String identifier) { | ||
| if (identifier == null || identifier.length() < 2) { | ||
| return identifier; | ||
| } | ||
| char start = identifier.charAt(0); | ||
| char end = identifier.charAt(identifier.length() - 1); | ||
| final char closing; | ||
| if (start == '"' || start == '`') { | ||
| closing = start; | ||
| } else if (start == '[') { | ||
| closing = ']'; | ||
| } else { | ||
| return identifier; | ||
| } | ||
| if (end != closing) { | ||
| return identifier; | ||
| } | ||
| String inner = identifier.substring(1, identifier.length() - 1); | ||
| // A literal quote char inside a quoted identifier is escaped by doubling it. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also works for SQL Server, mysql, and mariadb. How about |
||
| return inner.replace(String.valueOf(closing) + closing, String.valueOf(closing)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| --! qt:database:mariadb:qdb:q_test_case_sensitive_country_table.mariadb.sql | ||
|
|
||
| CREATE EXTERNAL TABLE country_lower (id int, name varchar(20)) | ||
| STORED BY 'org.apache.hive.storage.jdbc.JdbcStorageHandler' | ||
| TBLPROPERTIES ( | ||
| "hive.sql.database.type" = "MYSQL", | ||
| "hive.sql.jdbc.driver" = "org.mariadb.jdbc.Driver", | ||
| "hive.sql.jdbc.url" = "jdbc:mariadb://${system:hive.test.database.qdb.host}:${system:hive.test.database.qdb.port}/bob", | ||
| "hive.sql.dbcp.username" = "${system:hive.test.database.qdb.jdbc.username}", | ||
| "hive.sql.dbcp.password" = "${system:hive.test.database.qdb.jdbc.password}", | ||
| "hive.sql.table" = "country"); | ||
|
|
||
| EXPLAIN CBO SELECT COUNT(*) FROM country_lower; | ||
| SELECT COUNT(*) FROM country_lower; | ||
|
|
||
| -- The back-tick quoted mixed-case table must resolve to `Country` (2 rows). | ||
| CREATE EXTERNAL TABLE country_mixed (id int, name varchar(20)) | ||
| STORED BY 'org.apache.hive.storage.jdbc.JdbcStorageHandler' | ||
| TBLPROPERTIES ( | ||
| "hive.sql.database.type" = "MYSQL", | ||
| "hive.sql.jdbc.driver" = "org.mariadb.jdbc.Driver", | ||
| "hive.sql.jdbc.url" = "jdbc:mariadb://${system:hive.test.database.qdb.host}:${system:hive.test.database.qdb.port}/bob", | ||
| "hive.sql.dbcp.username" = "${system:hive.test.database.qdb.jdbc.username}", | ||
| "hive.sql.dbcp.password" = "${system:hive.test.database.qdb.jdbc.password}", | ||
| "hive.sql.table" = "`Country`"); | ||
|
|
||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.