[SPARK-52832][SQL] Fix JDBC dialect identifier quoting #51533
Conversation
urosstan-db
changed the title
| val mySQLDialect = JdbcDialects.get("jdbc:mysql://127.0.0.1/db") | ||
| val postgresDialect = JdbcDialects.get("jdbc:postgresql://127.0.0.1/db") | ||
| val derbyDialect = JdbcDialects.get("jdbc:derby:db") | ||
| val oracleDialect = JdbcDialects.get("jdbc:oracle:thin:@//localhost:1521/orcl") | ||
|
|
||
| val columns = Seq("abc", "key") |
There was a problem hiding this comment.
Can we put "col\"" here to test all dialects?
There was a problem hiding this comment.
Yes, but many of them currently does not handle escaping properly (except the ones where quotes can't be part of identifiers).
I would rather fix one by one, or in smaller batches than to scope up PR and make some mistake.
I am fine also with making default implementation like Oracle's implementation, since if certain dialect use double quotes for quoting, but use some other escape character, we won't break anything, since currently escaping does not work at all.
There was a problem hiding this comment.
yea I think we should make it the default, as it won't break anything.
There was a problem hiding this comment.
Agreed, I checked almost all dialects, it seems we are fine with this rule of escaping. The only one that may not work are dialects out of Spark core project, but those don't work already.
urosstan-db
changed the title
…dentifier-quoting' into SPARK-52832-fix-oracle-dialect-identifier-quoting # Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
…2-fix-oracle-dialect-identifier-quoting
| assertResult(Seq("\"abc\"", "\"key\"", "\"double_quote\"\"\"", "\"back`\""))(postgresColumns) | ||
| assertResult(Seq("\"abc\"", "\"key\"", "\"double_quote\"\"\"", "\"back`\""))(derbyColumns) | ||
| assertResult(Seq("\"abc\"", "\"key\"", "\"double_quote\"\"\"", "\"back`\""))(oracleColumns) | ||
| assertResult(Seq("`abc`", "`key`", "`double_quote\"`", "`back```"))(databricksColumns) | ||
| } |
There was a problem hiding this comment.
assertResult is used because it gives better error message, it accepts expected and actual parameters, so it can tell what is expected values in error message.a
| @@ -286,6 +286,11 @@ private case class OracleDialect() extends JdbcDialect with SQLConfHelper with N | |||
| case _ => super.classifyException(e, condition, messageParameters, description, isRuntime) | |||
| } | |||
| } | |||
|
|
|||
| override def quoteIdentifier(colName: String): String = { | |||
There was a problem hiding this comment.
we can remove it now as it's the same as default.
What changes were proposed in this pull request?
JDBCDialectMySQLDialectescapes backticks with another backticksDatabricksDialectescapes backticks with another backticksWhy are the changes needed?
Resolving a bug when JDBC connector needs to quote an identifier when identifier contains double quotes/backticks.
Does this PR introduce any user-facing change?
Fix identifier quoting in JDBC dialect when identifier contains double quotes/backticks.
How was this patch tested?
Using JDBCSuite
Was this patch authored or co-authored using generative AI tooling?
No