Enable testColumnName in TestCassandraDistributedQueries #3623
Conversation
ebyhr
force-pushed
the
cassandra-name-test
branch
3 times, most recently
from
ac4d3f3
to
5ca95a9
Compare
| { | ||
| String randomSuffix = Long.toString(abs(random.nextLong()), MAX_RADIX); | ||
| return randomSuffix.substring(0, min(RANDOM_SUFFIX_LENGTH, randomSuffix.length())); | ||
| return randomSuffix.substring(0, min(maxLength, randomSuffix.length())); |
There was a problem hiding this comment.
can we simply change the default here? 12 was quite big.
| @@ -289,7 +290,7 @@ private CassandraOutputTableHandle createTable(ConnectorTableMetadata tableMetad | |||
| for (ColumnMetadata column : tableMetadata.getColumns()) { | |||
| columnNames.add(column.getName()); | |||
| columnTypes.add(column.getType()); | |||
| columnExtra.add(new ExtraColumnMetadata(column.getName(), column.isHidden())); | |||
| columnExtra.add(new ExtraColumnMetadata(escapeSingleQuote(column.getName()), column.isHidden())); | |||
There was a problem hiding this comment.
Escaping should be done when we construct a CQL query
| { | ||
| return '"' + identifier + '"'; | ||
| return identifier.replace("'", "''"); |
There was a problem hiding this comment.
I am concerned about escape function that doesn't quote.
Generally, escaping and quoting should be done at the same time, so they should be done
at once, in one function.
| @@ -94,6 +93,11 @@ protected boolean supportsArrays() | |||
| return true; | |||
| } | |||
|
|
|||
| protected String randomTableSuffix() | |||
There was a problem hiding this comment.
Please remove, as per @kokosing's https://github.com/prestosql/presto/pull/3623/files#r419861858 comment
| @@ -27,7 +27,7 @@ | |||
| implements AutoCloseable | |||
| { | |||
| private static final SecureRandom random = new SecureRandom(); | |||
| private static final int RANDOM_SUFFIX_LENGTH = 12; | |||
| private static final int RANDOM_SUFFIX_LENGTH = 6; | |||
There was a problem hiding this comment.
about 2^31 options, but since we generate them a lot.... can this be eg 8?
we can shorted the table name's base a bit maybe?
There was a problem hiding this comment.
Currently, only 0startingwithdigit is the problem. Let me rename to 0startwithdigit. and drop the commit to shorten.
| @@ -1180,7 +1180,7 @@ private void testColumnName(String columnName, boolean delimited) | |||
|
|
|||
| try { | |||
| // TODO test with both CTAS *and* CREATE TABLE + INSERT, since they use different connector API methods. | |||
| assertUpdate("CREATE TABLE " + tableName + "(id varchar, " + nameInSql + " varchar)"); | |||
| assertUpdate("CREATE TABLE " + tableName + "(key varchar, " + nameInSql + " varchar)"); | |||
There was a problem hiding this comment.
Rename id column to key in testColumnName
in the commit message maybe mention that id has a special meaning in Cassandra.
| @@ -150,16 +150,6 @@ protected TestTable createTableWithDefaultColumns() | |||
| throw new SkipException("Cassandra connector does not support column default values"); | |||
| } | |||
|
|
|||
| @Override | |||
| public void testColumnName(String columnName) | |||
There was a problem hiding this comment.
Enable testColumnName in TestCassandraDistributedQueries
Consider squashing this commit with Use Metadata.quote in CassandraCqlUtils -- if this is what fixes Cassandra and makes this test pass now.
The id column name is reserved in Cassandra connector. Replace 0startingwithdigit with 0startwithdigit to avoid column name length limitation (48) in Cassandra.
To use escaping logic of Metadata.quote method. Additionally, escape single quotation for table comment and enable testColumnName in Cassandra.
|
CI failure: #3635 |
This requires changes of AbstractTestDistributedQueries.
idis automatically generated in Cassandra connector(Ongoing Oracle PR's column length limitation is 30 due to the image version)