Skip to content
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 YugabyteDB connector #5352

Open
4 tasks
ebyhr opened this issue Sep 30, 2020 · 26 comments
Open
4 tasks

Add support for YugabyteDB connector #5352

ebyhr opened this issue Sep 30, 2020 · 26 comments
Labels
enhancement New feature or request

Comments

@ebyhr
Copy link
Member

ebyhr commented Sep 30, 2020

Yugabyte offers JDBC drivers, but it has BETA label for now.
https://docs.yugabyte.com/latest/reference/drivers/yugabytedb-jdbc-driver/

Another option is using Java client driver that is based on DataStax's one (similar to the current Cassandra connector).
https://docs.yugabyte.com/latest/reference/drivers/ycql-client-drivers/#java

Related to #4836 #4838

Reported issue:

@tedyu
Copy link

tedyu commented Sep 30, 2020

Suggested change for presto-cassandra-driver to connect to Yugabyte DB:

diff --git a/pom.xml b/pom.xml
index c0c71c2..ebf25d9 100644
--- a/pom.xml
+++ b/pom.xml
@@ -41,11 +41,17 @@

     <dependencies>
         <dependency>
-            <groupId>com.datastax.cassandra</groupId>
+            <groupId>com.yugabyte</groupId>
             <artifactId>cassandra-driver-core</artifactId>
-            <version>3.6.0</version>
+            <version>3.2.0-yb-19</version>
             <scope>runtime</scope>
         </dependency>
+            <dependency>
+                <groupId>com.google.guava</groupId>
+                <artifactId>guava</artifactId>
+                <version>19.0</version>
+              <scope>runtime</scope>
+            </dependency>
     </dependencies>

     <build>
@@ -87,7 +93,7 @@
                             <promoteTransitiveDependencies>true</promoteTransitiveDependencies>
                             <artifactSet>
                                 <includes>
-                                    <include>com.datastax.cassandra:cassandra-driver-core</include>
+                                    <include>com.yugabyte:cassandra-driver-core</include>
                                     <include>com.google.guava:guava</include>
                                     <include>org.ow2.asm:*</include>
                                     <include>com.github.jnr:*</include>

@ebyhr
Copy link
Member Author

ebyhr commented Sep 30, 2020

@tedyu I don't think we can replace the dependency. We will need to create a new connector.

@tedyu
Copy link

tedyu commented Sep 30, 2020

Agreed to creating new connector for Presto.

The Yugabyte JDBC driver covers both YCQL (for Cassandra) and YSQL (for Postgres).
The Beta label is for some YSQL feature to be added, not for YCQL.

For YCQL, Presto should be able to use Yugabyte JDBC driver.

@tedyu
Copy link

tedyu commented Sep 30, 2020

@ebyhr
Do you mind creating a new connector for Yugabyte DB (via Yugabyte JDBC driver) ?

Thanks

@findepi
Copy link
Member

findepi commented Oct 2, 2020

Do you mind creating a new connector for Yugabyte DB (via Yugabyte JDBC driver) ?

Since Yugabyte is different from Cassandra, yes we should have dedicated connector.
Does Yugabyte aims for maintaining JDBC and behavior compatibility with Cassandra (also for future Cassandra changes)
If yes, we could perhaps consider creating Yugabyte connector which extends Cassandra connector.
We should also consider copying the connector -- as we did for eg MemSQL (#1906), since MemSQL does not apparently aim at maintaining compat with MySQL.

@tedyu do you want to work on this?

@ebyhr @tedyu @electrum let's have agreement before commencing the work.

@tedyu
Copy link

tedyu commented Oct 2, 2020

Yugabyte does plan to maintain JDBC and behavior compatibility with Cassandra.

The suggested change in #5352 (comment) references cassandra-driver-core maintained by yugabyte.
This is for the Presto Yugabyte connector (to be created in new repo).

@tedyu
Copy link

tedyu commented Oct 9, 2020

@ebyhr @findepi

Given the change in:
#5352 (comment)

Can you help create repository for Presto connector ?

Thanks

@findepi
Copy link
Member

findepi commented Oct 9, 2020

@tedyu i am not sure what kind of repository this would be.
All the supported connectors are directly in this repository, as maven modules.
(https://github.com/prestosql/presto-cassandra-driver exists for technical reasons, this is not a Presto plugin)

@tedyu
Copy link

tedyu commented Oct 9, 2020

I was expecting https://github.com/prestosql/presto-yugabytedb-driver or something like that to be created.

Thanks

@findepi
Copy link
Member

findepi commented Oct 10, 2020

@tedyu presto-cassandra-driver exists because we need to re-package the driver, shading-in some dependencies.
Is it also the case for Yugabyte driver? what are its dependencies?

@tedyu
Copy link

tedyu commented Oct 10, 2020

yb-cassandra-dep.txt

Please see the attached file (up to line 76)

@findepi
Copy link
Member

findepi commented Oct 12, 2020

is Yugabyte driver a fork of cassandra driver?
i think the guava (v 19) dep can be a problem.
OK, i created a https://github.com/prestosql/presto-yugabyte-driver repo.

@findepi
Copy link
Member

findepi commented Oct 13, 2020

@tedyu can you please created a companion PR here adding the new module?
also, please make sure to sign the CLA

@tedyu
Copy link

tedyu commented Oct 13, 2020

For the companion PR, wouldn't it be dependent on the availability of presto-yugabyte-driver in maven ?

Signature has been sent to cla@prestosql.io. Is there more to be done in this regard ?

@findepi
Copy link
Member

findepi commented Oct 16, 2020

Signature has been sent to cla@prestosql.io. Is there more to be done in this regard ?

cc @martint

For the companion PR, wouldn't it be dependent on the availability of presto-yugabyte-driver in maven ?

the PR won't build on CI until presto-yugabyte-driver is published,
but will still allow a reviewer to build both and verify that what we have in trinodb/trino-yugabyte-driver#1 is sufficient

the next step would be to merge trinodb/trino-yugabyte-driver#1, release do maven, etc.

@tedyu
Copy link

tedyu commented Oct 16, 2020

The following would allow building against presto-yugabyte-driver :

diff --git a/pom.xml b/pom.xml
index a275578596..f3e9794cc7 100644
--- a/pom.xml
+++ b/pom.xml
@@ -1727,6 +1727,18 @@
                 </exclusions>
             </dependency>

+            <dependency>
+                <groupId>com.facebook.presto.yugabyte</groupId>
+                <artifactId>yugabyte-driver</artifactId>
+                <version>3.6.0-2-SNAPSHOT</version>
+                <exclusions>
+                    <exclusion>
+                        <groupId>io.netty</groupId>
+                        <artifactId>*</artifactId>
+                    </exclusion>
+                </exclusions>
+            </dependency>
+
             <dependency>
                 <groupId>com.facebook.presto.pinot</groupId>
                 <artifactId>pinot-driver</artifactId>
diff --git a/presto-cassandra/pom.xml b/presto-cassandra/pom.xml
index ecd26c5512..ae4678dca4 100644
--- a/presto-cassandra/pom.xml
+++ b/presto-cassandra/pom.xml
@@ -17,8 +17,8 @@

     <dependencies>
         <dependency>
-            <groupId>com.facebook.presto.cassandra</groupId>
-            <artifactId>cassandra-driver</artifactId>
+            <groupId>com.facebook.presto.yugabyte</groupId>
+            <artifactId>yugabyte-driver</artifactId>
         </dependency>

         <dependency>

Do you have suggestion other than copying the whole presto-cassandra subproject ?

Thanks

@findepi
Copy link
Member

findepi commented Oct 20, 2020

Do you have suggestion other than copying the whole presto-cassandra subproject ?

Let's start simpler:

  • create new presto-yugabyte module
  • have presto-cassandra as a dependency
  • exclude cassandra driver and add yugabyte driver

@tedyu
Copy link

tedyu commented Oct 21, 2020

yuga-module.txt

See if the patch is on right track.

Currently trying to figure out which Enforcer rule fails:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M2:enforce (default) on project presto-yugabyte: Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed. -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M2:enforce (default) on project presto-yugabyte: Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed.

@findepi
Copy link
Member

findepi commented Oct 26, 2020

yuga-module.txt

See if the patch is on right track.

instead of a diff, why don't you make a PR?
it will be much easier to work with, comment, etc.

@tedyu
Copy link

tedyu commented Oct 26, 2020

Even with -X option, I didn't get more clue on which enforcer rule was broken.

Once the compilation passes, I would send out the PR.

Meanwhile, suggestion on how to fix compilation is welcome.

@findepi
Copy link
Member

findepi commented Oct 26, 2020

Meanwhile, suggestion on how to fix compilation is welcome.

let's see how it looks like in the PR.

@tedyu
Copy link

tedyu commented Oct 26, 2020

prestodb/presto#15357

@ebyhr
Copy link
Member Author

ebyhr commented Oct 27, 2020

@tedyu It seems you sent PR to different repository. Could you resend to prestosql/presto?

@tedyu
Copy link

tedyu commented Oct 27, 2020

#5708

@ebyhr
Copy link
Member Author

ebyhr commented Aug 15, 2021

Yugabyte (CQL) doesn't support setting a comment in both CREATE TABLE and ALTER TABLE.
https://docs.yugabyte.com/latest/api/ycql/ddl_create_table/#table-properties-1

This limitation breaks INSERT statement and affects to some operations because Cassandra connector uses the comment to manage hidden columns.

@tedyu
Copy link

tedyu commented Sep 1, 2021

I logged yugabyte/yugabyte-db#9902 for comment enhancement in YCQL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

3 participants