Quote column name in Phoenix CREATE TABLE statement #3601
Conversation
| throw new SkipException("TODO"); | ||
| if (columnName.equals("a\"quote")) { | ||
| assertThatThrownBy(() -> super.testColumnName(columnName)) | ||
| .hasMessageContaining("Illegal data. Unsupported sql type: QUOTE"); |
There was a problem hiding this comment.
That's incorrect error message. We didn't ask to create a column of type quote.
This is because getEscapedArgument doesn't support escaping " within the name.
If Phoenix does not support this, we need to filter this out on our side.
Then, the correct way to exclude this test case would be like here
https://github.com/prestosql/presto/blob/329a48592ecb6c628712ef3648f2e8da7eb33d38/presto-mysql/src/test/java/io/prestosql/plugin/mysql/TestMySqlDistributedQueries.java#L123-L127
There was a problem hiding this comment.
@findepi Replaced with isColumnNameRejected. As far as I confirmed, we cannot use double-quotation for column name.
There was a problem hiding this comment.
@ebyhr thanks!
please make sure the column name is cleanly rejected at some point, so that user knows this is illegal column name, not something else.
eg io.prestosql.plugin.mysql.TestMySqlDistributedQueries#isColumnNameRejected verifies the actual exception message contains "Incorrect column name"
presto-phoenix/src/main/java/io/prestosql/plugin/phoenix/PhoenixMetadata.java
Outdated
Show resolved
Hide resolved
| { | ||
| // TODO (https://github.com/prestosql/presto/issues/3466) Phoenix generally lacks quoting in underlying queries | ||
| throw new SkipException("TODO"); | ||
| return columnName.equals("a\"quote"); |
There was a problem hiding this comment.
Leave some TODO comment still.
Currently the " in column name leads to SQL injection.
Use of a"quote column should produce a reasonable exception message like "Invalid column name" or "Column name cannot contain a quotation sign". Then, here you should verify the actual exception message.
Additionally, enable testColumnName test in Phoenix.
Related to #3466