[SPARK-57447][SQL] Escape the index name in (H2|MySQL|Postgres)Dialect.indexExists#56508
[SPARK-57447][SQL] Escape the index name in (H2|MySQL|Postgres)Dialect.indexExists#56508dongjoon-hyun wants to merge 1 commit into
(H2|MySQL|Postgres)Dialect.indexExists#56508Conversation
(H2|MySQL|Postgres)Dialect.indexExists(H2|MySQL|Postgres)Dialect.indexExists
|
cc @huaxingao |
|
All tests passed. |
|
Could you review this PR when you have some time, please, @viirya ? |
viirya
left a comment
There was a problem hiding this comment.
The fix itself is correct — escapeSql doubles single quotes, the three call sites all interpolate indexName inside a '...' literal, and I confirmed JdbcUtils.checkIfIndexExists swallows the exception and returns false, so the false-negative described in the PR is real. The Mockito test is nice: it captures the executed SQL and asserts the escaped form, so all three dialects are covered in plain CI without a live DB.
My one concern is scope vs. the bug class. The same false-negative pattern still exists in these methods for the other interpolated literals:
H2Dialect.indexExistsstill hasTABLE_SCHEMA = '${tableIdent.namespace().last}'andTABLE_NAME = '${tableIdent.name()}'unescaped.PostgresDialect.indexExistsstill hastablename = '${tableIdent.name()}'unescaped.
A table named o'brien reproduces the identical malformed-query -> swallowed-exception -> false "index does not exist" behavior. The same raw-literal pattern also appears in listIndexes (H2Dialect and PostgresDialect build ... = '${tableIdent.name()}' the same way).
Could we either escape the table/schema names in these queries too, or note in the PR description that they're intentionally out of scope for a follow-up? Leaning toward fixing them here since it's just a few more escapeSql calls and avoids a near-identical bug report later. Not blocking — the change as-is is safe and correct.
|
Thank you, @viirya and @huaxingao . To @viirya , I revise the PR description by mentioning the scope is only
|
…ct.indexExists` ### What changes were proposed in this pull request? `indexExists` in `H2Dialect`, `MySQLDialect`, and `PostgresDialect` embeds the index name as a SQL string literal in its lookup query. This PR wraps the index name with the existing `JdbcDialect.escapeSql` helper so that single quotes are properly escaped. Note that the scope of this PR is only index names. For table names or TABLE_SCHEMA, we are going to handle independently. ### Why are the changes needed? If an index name contains a single quote, the lookup query becomes malformed (e.g. `... INDEX_NAME = 'a'b'`). `JdbcUtils.checkIfIndexExists` swallows the resulting exception and returns `false`, so an existing index whose name contains a single quote is incorrectly reported as not existing. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes #56508 from dongjoon-hyun/SPARK-57447. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 86db727) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
|
Merged to master/4.x for now because we have Apache Spark 4.2.0 RC3 vote currently. If RC3 fails, I will consider to cherry-pick this to |
…ct.indexExists` ### What changes were proposed in this pull request? `indexExists` in `H2Dialect`, `MySQLDialect`, and `PostgresDialect` embeds the index name as a SQL string literal in its lookup query. This PR wraps the index name with the existing `JdbcDialect.escapeSql` helper so that single quotes are properly escaped. Note that the scope of this PR is only index names. For table names or TABLE_SCHEMA, we are going to handle independently. ### Why are the changes needed? If an index name contains a single quote, the lookup query becomes malformed (e.g. `... INDEX_NAME = 'a'b'`). `JdbcUtils.checkIfIndexExists` swallows the resulting exception and returns `false`, so an existing index whose name contains a single quote is incorrectly reported as not existing. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes #56508 from dongjoon-hyun/SPARK-57447. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 86db727) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit ab289d6) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…ct.indexExists` `indexExists` in `H2Dialect`, `MySQLDialect`, and `PostgresDialect` embeds the index name as a SQL string literal in its lookup query. This PR wraps the index name with the existing `JdbcDialect.escapeSql` helper so that single quotes are properly escaped. Note that the scope of this PR is only index names. For table names or TABLE_SCHEMA, we are going to handle independently. If an index name contains a single quote, the lookup query becomes malformed (e.g. `... INDEX_NAME = 'a'b'`). `JdbcUtils.checkIfIndexExists` swallows the resulting exception and returns `false`, so an existing index whose name contains a single quote is incorrectly reported as not existing. No. Pass the CIs. Generated-by: Claude Code Closes #56508 from dongjoon-hyun/SPARK-57447. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 86db727) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit ab289d6) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 85a0972) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…ct.indexExists` `indexExists` in `H2Dialect`, `MySQLDialect`, and `PostgresDialect` embeds the index name as a SQL string literal in its lookup query. This PR wraps the index name with the existing `JdbcDialect.escapeSql` helper so that single quotes are properly escaped. Note that the scope of this PR is only index names. For table names or TABLE_SCHEMA, we are going to handle independently. If an index name contains a single quote, the lookup query becomes malformed (e.g. `... INDEX_NAME = 'a'b'`). `JdbcUtils.checkIfIndexExists` swallows the resulting exception and returns `false`, so an existing index whose name contains a single quote is incorrectly reported as not existing. No. Pass the CIs. Generated-by: Claude Code Closes #56508 from dongjoon-hyun/SPARK-57447. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 86db727) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit ab289d6) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 85a0972) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit c697510) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…ct.indexExists` `indexExists` in `H2Dialect`, `MySQLDialect`, and `PostgresDialect` embeds the index name as a SQL string literal in its lookup query. This PR wraps the index name with the existing `JdbcDialect.escapeSql` helper so that single quotes are properly escaped. Note that the scope of this PR is only index names. For table names or TABLE_SCHEMA, we are going to handle independently. If an index name contains a single quote, the lookup query becomes malformed (e.g. `... INDEX_NAME = 'a'b'`). `JdbcUtils.checkIfIndexExists` swallows the resulting exception and returns `false`, so an existing index whose name contains a single quote is incorrectly reported as not existing. No. Pass the CIs. Generated-by: Claude Code Closes #56508 from dongjoon-hyun/SPARK-57447. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 86db727) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit ab289d6) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 85a0972) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit c697510) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit da40fdb) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…ct.indexExists` ### What changes were proposed in this pull request? `indexExists` in `H2Dialect`, `MySQLDialect`, and `PostgresDialect` embeds the index name as a SQL string literal in its lookup query. This PR wraps the index name with the existing `JdbcDialect.escapeSql` helper so that single quotes are properly escaped. Note that the scope of this PR is only index names. For table names or TABLE_SCHEMA, we are going to handle independently. ### Why are the changes needed? If an index name contains a single quote, the lookup query becomes malformed (e.g. `... INDEX_NAME = 'a'b'`). `JdbcUtils.checkIfIndexExists` swallows the resulting exception and returns `false`, so an existing index whose name contains a single quote is incorrectly reported as not existing. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes apache#56508 from dongjoon-hyun/SPARK-57447. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
indexExistsinH2Dialect,MySQLDialect, andPostgresDialectembeds theindex name as a SQL string literal in its lookup query. This PR wraps the index
name with the existing
JdbcDialect.escapeSqlhelper so that single quotes areproperly escaped.
Note that the scope of this PR is only index names. For table names or TABLE_SCHEMA, we are going to handle independently.
Why are the changes needed?
If an index name contains a single quote, the lookup query becomes malformed
(e.g.
... INDEX_NAME = 'a'b').JdbcUtils.checkIfIndexExistsswallows theresulting exception and returns
false, so an existing index whose namecontains a single quote is incorrectly reported as not existing.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass the CIs.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code