-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Support smallint, tinyint, date type of Cassandra #141
Conversation
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.
Overall looks good, thank you for your contribution!
Bunch of comments.
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraMetadata.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraMetadata.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraMetadata.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraPageSink.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraPageSink.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Outdated
Show resolved
Hide resolved
...o-product-tests/src/main/java/io/prestosql/tests/cassandra/TestInsertIntoCassandraTable.java
Outdated
Show resolved
Hide resolved
664d997
to
9277d61
Compare
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Outdated
Show resolved
Hide resolved
@@ -76,6 +84,10 @@ public CassandraPageSink( | |||
this.columnTypes = ImmutableList.copyOf(requireNonNull(columnTypes, "columnTypes is null")); | |||
this.generateUUID = generateUUID; | |||
|
|||
toCassandraDate = protocolVersion.toInt() <= ProtocolVersion.V3.toInt() ? |
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.
do we have coverage for both protocols?
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.
As cassandra.protocol-version
in cassandra.rst, it says we can use V2, V3 and V4. Current upstream's test coverage is only V3 though.
Update variable name in CassandraType.java. |
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.
LGTM, minor comments.
Please apply changes inline (no fixup commits), but please do not rebase.
@@ -70,6 +71,7 @@ | |||
private final CassandraSession cassandraSession; | |||
private final CassandraPartitionManager partitionManager; | |||
private final boolean allowDropTable; | |||
private final ProtocolVersion protocolVersion; |
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.
cmt msg "Support smallint, tinyint, date type of Cassandra"
change to
Support Cassandra smallint, tinyint and date types
@@ -85,6 +87,7 @@ public CassandraMetadata( | |||
this.partitionManager = requireNonNull(partitionManager, "partitionManager is null"); | |||
this.cassandraSession = requireNonNull(cassandraSession, "cassandraSession is null"); | |||
this.allowDropTable = requireNonNull(config, "config is null").getAllowDropTable(); | |||
this.protocolVersion = requireNonNull(config, "config is null").getProtocolVersion(); |
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.
Annotate io.prestosql.plugin.cassandra.CassandraClientConfig#getProtocolVersion
with @NotNull
.
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraPageSink.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Outdated
Show resolved
Hide resolved
presto-cassandra/src/main/java/io/prestosql/plugin/cassandra/CassandraType.java
Outdated
Show resolved
Hide resolved
@findepi Sorry, what do you mean...? |
(discussed offline) |
4504eef
to
ee5cfe9
Compare
Rebased to resolve conflicts, therefore force-pushed shows large diff. |
@ebyhr thanks! |
ee5cfe9
to
b6e28b5
Compare
Squashed and the build is green. |
Merged, thanks! |
Cherry-pick of trinodb/trino#141. This is also a fix for prestodb#15147 which tried to backport Trino prestodb#141 but that backporting was incomplete and caused the Cassandra tests to fail. Resolves: prestodb#15749 Co-authored-by: Yuya Ebihara <ebyhry@gmail.com>
Cherry-pick of trinodb/trino#141. This is also a fix for #15147 which tried to backport Trino #141 but that backporting was incomplete and caused the Cassandra tests to fail. Resolves: #15749 Co-authored-by: Yuya Ebihara <ebyhry@gmail.com>
Continuation of prestodb/presto#7123