thomasrebele commented on code in PR #6727:
URL: https://github.com/apache/hive/pull/6727#discussion_r4534356507
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java:
##########
@@ -902,4 +903,47 @@ private Set<String> getResultPathes(List<Path>
foundManifestFiles) {
}
return resultPathes;
}
+
+ @Test
+ public void testUnquoteJdbcIdentifierNullAndShortInputsUnchanged() {
+ assertNull(Utilities.unquoteJdbcIdentifier(null));
+ assertEquals("", Utilities.unquoteJdbcIdentifier(""));
+ // A single character cannot be a matched quote pair.
+ assertEquals("\"", Utilities.unquoteJdbcIdentifier("\""));
+ }
+
+ @Test
+ public void testUnquoteJdbcIdentifierUnquotedInputUnchanged() {
+ // Unquoted identifiers keep the current (case-insensitive) behaviour.
Review Comment:
Shouldn't it say `(case-sensitive) behavior`? The expected values of the
three asserts are different, and I would call this behavior case-sensitive.
Similar to https://github.com/apache/hive/pull/6727/changes#r4534412083, I
would avoid referring to the behavior before merging the PR. How about
`Unquoted identifiers are handled in a case-sensitive way`?
##########
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:
I think it would be good to create a ticket for this task to avoid
forgetting it. Wdyt?
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java:
##########
@@ -902,4 +903,47 @@ private Set<String> getResultPathes(List<Path>
foundManifestFiles) {
}
return resultPathes;
}
+
+ @Test
+ public void testUnquoteJdbcIdentifierNullAndShortInputsUnchanged() {
+ assertNull(Utilities.unquoteJdbcIdentifier(null));
+ assertEquals("", Utilities.unquoteJdbcIdentifier(""));
+ // A single character cannot be a matched quote pair.
+ assertEquals("\"", Utilities.unquoteJdbcIdentifier("\""));
+ }
+
+ @Test
+ public void testUnquoteJdbcIdentifierUnquotedInputUnchanged() {
+ // Unquoted identifiers keep the current (case-insensitive) behaviour.
+ assertEquals("Country", Utilities.unquoteJdbcIdentifier("Country"));
+ assertEquals("country", Utilities.unquoteJdbcIdentifier("country"));
+ assertEquals("COUNTRY", Utilities.unquoteJdbcIdentifier("COUNTRY"));
+ }
+
+ @Test
+ public void testUnquoteJdbcIdentifierStripsSupportedQuotePairs() {
+ // ANSI / Oracle / Postgres double quotes.
+ assertEquals("Country", Utilities.unquoteJdbcIdentifier("\"Country\""));
+ // MySQL / MariaDB back-ticks.
+ assertEquals("Country", Utilities.unquoteJdbcIdentifier("`Country`"));
+ // SQL Server brackets.
+ assertEquals("Country", Utilities.unquoteJdbcIdentifier("[Country]"));
+ // Schema-style identifiers with mixed case are preserved after unquoting.
+ assertEquals("WorldData",
Utilities.unquoteJdbcIdentifier("\"WorldData\""));
Review Comment:
In case there is a DB that accepts these special characters to be part of
the regular table name, this could lead to a collision (e.g., if Postgres would
accept a table name `[Country]` then removing the brackets would lead to a
collision with `Country`). I vaguely remember having checked this during my
last review, and as far as I can recall, none of the DBs accepted such table
names.
Might be worth to check verify this, and add a comment to the
unquoteJdbcIdentifier 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:
[Resolved](https://github.com/orgs/community/discussions/19206). Thanks!
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcStorageHandler.java:
##########
@@ -107,10 +111,17 @@ 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);
+ String tableName = tableProperties.get(Constants.JDBC_TABLE);
+ // Tables defined through hive.sql.query (e.g. the sys and
information_schema tables) have no
+ // hive.sql.table, so their URI stays as before.
Review Comment:
The reader of the comment would understand the "as before" only if they look
at the code before merging the PR. In general I prefer comments that explain
the code as it is. Maybe something like this:
```
// Tables defined through hive.sql.query (e.g. the sys and
information_schema tables) have no
// hive.sql.table. They are represented by a pseudo-uri ending in
"/null".
```
An explanation why it is ok that they get a `.../null` auth URI would be
nice (I'm not an expert here).
In the case the purpose of the code cannot be understood anymore, it would
be better to write `so their URI stays as they were before merging 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.
+ 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:
Ok for me, let's see what other reviewers think.
##########
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:
[Resolved](https://github.com/orgs/community/discussions/19206). Thanks!
##########
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:
[Resolved](https://github.com/orgs/community/discussions/19206). Thanks!
--
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]
