Quote column names for `ADD`, `RENAME` and `DROP COLUMN` in JDBC connectors #13878

ebyhr · 2022-08-26T23:29:21Z

Description

Documentation

(x) No documentation is needed.

Release notes

(x) Release notes entries required with the following suggested text:

# ClickHouse, MariaDB, MySQL, Oracle, Phoenix, PostgreSQL, Redshift, SingleStore, SQL Server
* Fix query failure when renaming or dropping columns which should be quoted. ({issue}`13839`)

# Phoenix
* Fix query failure when adding a column which should be quoted. ({issue}`13839`)

plugin/trino-druid/src/test/java/io/trino/plugin/druid/BaseDruidConnectorTest.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlLegacyConnectorTest.java

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

findepi · 2022-08-29T10:45:35Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

+    }
+
+    @Test(dataProvider = "testColumnNameDataProvider")
+    public void testDropColumnName(String columnName)


Let's merge this with testAddColumnName, since the setup isn't trivial (CREATE + exception handling)

Do you mean testRenameColumnName? The setup in testAddColumnName creates a table without error handling.

I meant to merge this with testAddColumnName.

that would mean two lines more in that test

assertUpdate("ALTER TABLE " + tableName + " DROP COLUMN " + nameInSql); assertTableColumnNames(tableName, "value");

ws the whole boilerplate of this method.

You can also rename the test method to testAddAndDropColumnName

ebyhr · 2022-08-30T04:27:09Z

Addressed comments.

findepi · 2022-08-30T09:02:27Z

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

+
+    private static String escape(String name)
+    {
+        return name.replace("'", "'" + "'");


"'" + "'" -> "''"

findepi · 2022-08-30T09:04:16Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

+        }
+        assertTableColumnNames(tableName, columnName.toLowerCase(ENGLISH));
+
+        String newNameInSql = toColumnNameInSql(columnName + "_new", delimited);


we don't test column name with trailing spaces.

let's use columnName here without appending anything

let's use static a;b$c column name when creating the table in this test

findepi · 2022-08-30T09:05:22Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

+    }
+
+    @Test(dataProvider = "testColumnNameDataProvider")
+    public void testDropColumnName(String columnName)


I meant to merge this with testAddColumnName.

that would mean two lines more in that test

assertUpdate("ALTER TABLE " + tableName + " DROP COLUMN " + nameInSql); assertTableColumnNames(tableName, "value");

ws the whole boilerplate of this method.

You can also rename the test method to testAddAndDropColumnName

hashhar · 2022-09-10T10:42:06Z

SQL Server sp_rename doesn't behave as you'd expect.

Here's a test to showcase that:

In Trino (with this PR applied):

trino:dbo> create table dbo.test2 ("crazy; drop table customer" int, "[crazy; drop table customer]" int);
CREATE TABLE
trino:dbo> describe dbo.test2;
            Column            |  Type   | Extra | Comment 
------------------------------+---------+-------+---------
 crazy; drop table customer   | integer |       |         
 [crazy; drop table customer] | integer |       |         
(2 rows)

Query 20220910_104005_00030_ci9mm, FINISHED, 3 nodes
Splits: 17 total, 17 done (100.00%)
0.54 [2 rows, 152B] [3 rows/s, 280B/s]

trino:dbo> alter table dbo.test2 rename column "crazy; drop table customer" to "crazy; drop table nation";
RENAME COLUMN
trino:dbo> alter table dbo.test2 rename column "[crazy; drop table customer]" to "[crazy; drop table nation]";
Query 20220910_104038_00032_ci9mm failed: Unclosed quotation mark after the character string '[crazy; drop table nation], 'COLUMN''.

trino:dbo> describe dbo.test2;
            Column            |  Type   | Extra | Comment 
------------------------------+---------+-------+---------
 crazy; drop table nation     | integer |       |         
 [crazy; drop table customer] | integer |       |         
(2 rows)

Query 20220910_104042_00033_ci9mm, FINISHED, 3 nodes
Splits: 17 total, 17 done (100.00%)
0.46 [2 rows, 150B] [4 rows/s, 329B/s]

In SQL Server

CREATE TABLE dbo.test ([crazy; drop table customer] int, "[crazy; drop table customer]" int);
SELECT COLUMN_NAME FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_NAME = 'test';
-- crazy; drop table customer
-- [crazy; drop table customer]

EXEC sp_rename 'dbo.test.crazy; drop table customer', 'crazy; drop table nation', 'COLUMN';
EXEC sp_rename 'dbo.test.[crazy; drop table customer]', '[crazy; drop table nation]', 'COLUMN';
-- Errors out with SQL Error [15248] [S0001]: Either the parameter @objname is ambiguous or the claimed @objtype (COLUMN) is wrong.
EXEC sp_rename 'dbo.test."[crazy; drop table customer]"', '[crazy; drop table nation]', 'COLUMN';
SELECT COLUMN_NAME FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_NAME = 'test';
-- crazy; drop table nation
-- [crazy; drop table nation]

This suggests that the 2nd argument to sp_rename is treated literally. The 1st argument follows the rules for identifiers so [ and " are treated as special characters and act as delimiters instead of literal values.

hashhar · 2022-09-10T10:43:44Z

See also this in SQL Server:

CREATE TABLE dbo.test3 ([crazy" drop table customer] int, "[crazy drop table customer]" int);
SELECT COLUMN_NAME FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_NAME = 'test3';
-- crazy" drop table customer 
-- [crazy drop table customer]
EXEC sp_rename 'dbo.test3.[crazy" drop table customer]', '[crazy" drop table nation]', 'COLUMN';
EXEC sp_rename 'dbo.test3."[crazy drop table customer]"', '[crazy" drop table customer]', 'COLUMN';
SELECT COLUMN_NAME FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_NAME = 'test3';
-- [crazy" drop table nation]
-- [crazy" drop table customer]

hashhar · 2022-09-10T10:52:39Z

i.e. sp_rename treats the 2nd argument always literally but the first argument follows the identifier naming rules and hence needs quoting.

Also looks like we should delegate the work of doing the quoting to SQL Server itself using https://docs.microsoft.com/en-us/sql/t-sql/functions/quotename-transact-sql?redirectedfrom=MSDN&view=sql-server-ver16

Something like:

SELECT QUOTENAME('crazy; drop table customer');
SELECT QUOTENAME('[crazy; drop table customer');
SELECT QUOTENAME('[crazy; drop table customer]');
SELECT QUOTENAME('[crazy;" " drop table customer]');
SELECT QUOTENAME('"[crazy;" " drop table customer]"');
SELECT QUOTENAME('["[crazy;" " drop table customer]"]');

hashhar · 2022-09-12T08:09:31Z

I'll send a draft PR with the QUOTENAME approach and we can continue there.

ssheikin · 2022-09-13T19:35:09Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

+    }
+
+    @Test(dataProvider = "testColumnNameDataProvider")
+    public void testRenameColumnName(String columnName)


Why do we need this new method testRenameColumnName? Why cannot we use/update old one testRenameColumn? What is the principal difference between them? How the name of these methods describe that difference?

testRenameColumn should execute some operations to verify RENAME COLUMN works correctly (e.g. data didn't loss after renaming). testRenameColumnName should focus on test for column names. I separated these tests because the test purpose is different.

How the name of these methods describe that difference?

Please feel free to send PR if you feel the name isn't clear,

cla-bot bot added the cla-signed label Aug 26, 2022

ebyhr force-pushed the ebi/jdbc-quote-column-names branch from a4bd8fe to d34fa91 Compare August 27, 2022 01:45

ebyhr changed the title ~~Quote column names when renaming and dropping in JDBC connectors~~ Quote column names for ADD, RENAME and DROP COLUMN in JDBC connectors Aug 27, 2022

ebyhr force-pushed the ebi/jdbc-quote-column-names branch 3 times, most recently from 2c992e2 to 16534ae Compare August 29, 2022 06:43

ebyhr marked this pull request as ready for review August 29, 2022 09:48

ebyhr requested review from wendigo, findepi and hashhar August 29, 2022 09:49

findepi reviewed Aug 29, 2022

View reviewed changes

ebyhr force-pushed the ebi/jdbc-quote-column-names branch from 16534ae to ddb4fb4 Compare August 30, 2022 00:17

findepi and others added 2 commits August 30, 2022 09:19

Expect support for DROP column only when ADD is supported

83445df

Extract toColumnNameInSql method in BaseConnectorTest

36fc33e

ebyhr force-pushed the ebi/jdbc-quote-column-names branch 2 times, most recently from d3eef3e to 3e7d20c Compare August 30, 2022 01:30

findepi approved these changes Aug 30, 2022

View reviewed changes

findepi mentioned this pull request Aug 30, 2022

Expect support for DROP column only when ADD is supported #13897

Closed

ebyhr force-pushed the ebi/jdbc-quote-column-names branch 3 times, most recently from 1980096 to bc34280 Compare August 31, 2022 05:34

Quote column names for ADD, RENAME and DROP COLUMN in JDBC connectors

7306441

ebyhr force-pushed the ebi/jdbc-quote-column-names branch from bc34280 to 7306441 Compare August 31, 2022 05:40

ebyhr merged commit 3ebd9c2 into trinodb:master Aug 31, 2022

ebyhr deleted the ebi/jdbc-quote-column-names branch August 31, 2022 10:08

ebyhr mentioned this pull request Aug 31, 2022

Release notes for 395 #13913

Closed

github-actions bot added this to the 395 milestone Aug 31, 2022

colebow mentioned this pull request Sep 2, 2022

Add Trino 395 release notes #13975

Merged

ssheikin reviewed Sep 13, 2022

View reviewed changes

vlad-lyutenko mentioned this pull request Sep 23, 2022

Call sp_rename as prepared statement #14272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quote column names for `ADD`, `RENAME` and `DROP COLUMN` in JDBC connectors #13878

Quote column names for `ADD`, `RENAME` and `DROP COLUMN` in JDBC connectors #13878

ebyhr commented Aug 26, 2022 •

edited

Loading

findepi Aug 29, 2022

ebyhr Aug 30, 2022

findepi Aug 30, 2022

ebyhr commented Aug 30, 2022

findepi Aug 30, 2022

findepi Aug 30, 2022

findepi Aug 30, 2022

hashhar commented Sep 10, 2022

hashhar commented Sep 10, 2022 •

edited

Loading

hashhar commented Sep 10, 2022 •

edited

Loading

hashhar commented Sep 12, 2022

ssheikin Sep 13, 2022

ebyhr Sep 13, 2022 •

edited

Loading

ssheikin Sep 13, 2022

Quote column names for ADD, RENAME and DROP COLUMN in JDBC connectors #13878

Quote column names for ADD, RENAME and DROP COLUMN in JDBC connectors #13878

Conversation

ebyhr commented Aug 26, 2022 • edited Loading

Description

Documentation

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Aug 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar commented Sep 10, 2022

In SQL Server

hashhar commented Sep 10, 2022 • edited Loading

hashhar commented Sep 10, 2022 • edited Loading

hashhar commented Sep 12, 2022

Choose a reason for hiding this comment

ebyhr Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quote column names for `ADD`, `RENAME` and `DROP COLUMN` in JDBC connectors #13878

Quote column names for `ADD`, `RENAME` and `DROP COLUMN` in JDBC connectors #13878

ebyhr commented Aug 26, 2022 •

edited

Loading

hashhar commented Sep 10, 2022 •

edited

Loading

hashhar commented Sep 10, 2022 •

edited

Loading

ebyhr Sep 13, 2022 •

edited

Loading