thomasrebele commented on code in PR #6727:
URL: https://github.com/apache/hive/pull/6727#discussion_r3892991065
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:
##########
@@ -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.
Review Comment:
This also works for [SQL
Server](https://learn.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/ms176027(v=sql.105)?redirectedfrom=MSDN),
[mysql](https://dev.mysql.com/doc/refman/8.0/en/identifiers.html), and
[mariadb](https://mariadb.com/docs/server/reference/sql-structure/sql-language-structure/identifier-names).
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 `'`.
##########
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/TestJdbcStorageHandlerAuthUri.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * 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;
+
+/**
+ * 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());
Review Comment:
Shouldn't the JDBC URL contain the schema? I've found a [Stackoverflow
question](https://stackoverflow.com/questions/4168689/is-it-possible-to-specify-the-schema-when-connecting-to-postgres-with-jdbc).
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.
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcStorageHandler.java:
##########
@@ -107,10 +111,20 @@ public URI getURIForAuth(Table table) throws
URISyntaxException {
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.
Review 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.
##########
ql/src/test/queries/clientpositive/jdbc_case_sensitive_table_mysql.q:
##########
@@ -0,0 +1,30 @@
+--! qt:database:mysql:qdb:q_test_case_sensitive_country_table.mysql.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" = "com.mysql.jdbc.Driver",
+ "hive.sql.jdbc.url" = "${system:hive.test.database.qdb.jdbc.url}",
+ "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" = "com.mysql.jdbc.Driver",
+ "hive.sql.jdbc.url" = "${system:hive.test.database.qdb.jdbc.url}",
+ "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;
Review 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?
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcStorageHandler.java:
##########
@@ -107,10 +111,20 @@ public URI getURIForAuth(Table table) throws
URISyntaxException {
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);
+ }
+
+ private static String encodeIdentifierForAuth(String identifier) {
+ String physical = unquoteJdbcIdentifier(identifier);
+ if (physical == null) {
+ return null;
Review 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".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
