-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Cassandra tuple type #416
Add support for Cassandra tuple type #416
Conversation
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/test/java/io/prestosql/plugin/cassandra/CassandraTestingUtils.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/test/java/io/prestosql/plugin/cassandra/CassandraTestingUtils.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/test/java/io/prestosql/plugin/cassandra/TestCassandraConnector.java
Outdated
Show resolved
Hide resolved
...o-product-tests/src/main/java/io/prestosql/tests/cassandra/TestInsertIntoCassandraTable.java
Outdated
Show resolved
Hide resolved
row(1, "(1,1)")); | ||
|
||
assertThat(() -> query(format("INSERT INTO %s.%s.%s (key, value) VALUES (2, (2,2))", CONNECTOR_NAME, KEY_SPACE, tableName))) | ||
.failsWithMessage("Codec not found for requested operation: [frozen<test.type_tuple_insert> <-> java.lang.String]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: This exception message comes from Cassandra library. It would be better to throw more explicit message in seprated PR (other existing types are also the same).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will leave it as it is and will be addressed later by the separate PR .
presto-product-tests/src/main/java/io/prestosql/tests/cassandra/TestSelect.java
Outdated
Show resolved
Hide resolved
presto-product-tests/src/main/java/io/prestosql/tests/cassandra/TestSelect.java
Outdated
Show resolved
Hide resolved
|
||
query(format("INSERT INTO %s.%s.%s (key, value) VALUES (1, (1,1))", CONNECTOR_NAME, KEY_SPACE, tableName)); | ||
assertThat(query(format("SELECT * FROM %s.%s.%s", CONNECTOR_NAME, KEY_SPACE, tableName))).containsOnly( | ||
row(1, "(1,1)")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mapped tuple
to presto VARCHAR
, therefore the tuple value should be '(1,1)'
. Current (1,1)
is identified as ARRAY
What we want to check in this test method is INSERT fails like L185. L180-182 are not necessary.
Thank you for creating this PR! I left a few comments. If you want to run Ref: #415 |
@@ -211,6 +211,7 @@ TIMEUUID VARCHAR | |||
TINYINT TINYINT | |||
VARCHAR VARCHAR | |||
VARIANT VARCHAR | |||
TUPLE VARCHAR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also update L220.
Types not mentioned in the table above are not supported (e.g. tuple or UDT).
to
Types not mentioned in the table above are not supported (e.g. UDT).
@kokosing The main difference is UDT includes the column names like json, but tuple doesn't. Therefore, I thought mapping tuple to varchar is fine. tuple: If we map tuple to row type, perhaps we need to assign dummy column name like below. |
@ebyhr Note: currently, the fields will be show up as
|
@findepi Oh, I didn't know that. Which is better |
c499f78
to
1a06156
Compare
Yes, still it is much better than parsing |
@kokosing Sorry, I meant anonymous row |
@ebyhr, where do you see the "field0", "field1", etc. for anonymous rows? We removed all such usages a while ago, but we may have missed something. If you don't have field names, it's more natural to use You're right that using anonymous row types requires casting them to one with names before accessing the fields because the dereference operator is undefined in the other case in Presto. For a long time I've been thinking we might want to add support for positional access for row types (as an extension to standard SQL). This could easily be done by allowing row types to support the subscript operator: e.g., |
@martint that would be convenient. The current approach (cast to a named row, enumerating all types) is quite verbose, so I sympathise with a temptation to assign dummy field names. |
@martint I heard the "field0" for anonymous from @findepi #416 (comment). @leon-gh Sorry for consuing you. Let's use This is related issue about accessing the row field by index. |
OK, just updated the test now and will add a change to convert to RowType#anonymous |
That's the tough part. We would almost need to support dependent types and concepts that I'm not sure how we'd express in Presto today. In particular, given
What's the type of the |
@leon-gh Apologies, it looks like this slipped through the cracks. Now that we have easy access to fields in anonymous row types using the |
Sorry, been busy with other things. Sure, all I wanted was the C* tables to be supported, didn't really mind that much about the types - as long as I can export/import the data, that's all good. |
You need to create a block from the row type associated with that column and then append values to it. See this for an example: https://github.com/prestosql/presto/blob/master/presto-rcfile/src/main/java/io/prestosql/rcfile/text/BlockEncoding.java#L63-L73 |
Fixed via #8570 |
Added support for tuple Cassandra type, followed implementation for #147.