Add CockroachDB connector #8317
Conversation
|
And not surprisingly .. there are test failures in the module on the GA build.. |
| import static java.util.Objects.requireNonNull; | ||
| import static java.util.stream.Collectors.joining; | ||
|
|
||
| public class CockroachDbClient |
There was a problem hiding this comment.
Separate verbatim copy and mechanical renames from any other changes.
There was a problem hiding this comment.
Sorry.. what do you mean here?
There was a problem hiding this comment.
Most of this code seem to be a copy of PostgreSQL connector code and so there is no point in reviewing that again.
Most -- but not all. It would be good separate "copy" from "mechanical changes (renames)" and "any other changes", so that it's reviewable.
There was a problem hiding this comment.
Maybe we could treat postgresql connector as a library? instead of copying
There was a problem hiding this comment.
Let's see what are actual modifications. I would assume PostgreSQL and Cockroach not to converge in the future.
There was a problem hiding this comment.
I only used PostgreSQL connector as starting point since I know that CockroachDB is compatible and uses the PostgreSQL JDBC driver. I fully expect to change this code quite a bit to adjust to CockroachDB. It just provided a better starting point than a clean slate. My plan is to test it manually to see how far I get, then get the testcontainer setup to work and slowly work my way through all the tests and adjust as needed. It will only then make sense to really review.. hence the PR in draft..
plugin/trino-cockroachdb/src/main/java/io/trino/plugin/cockroachdb/CockroachDbClient.java
Outdated
Show resolved
Hide resolved
.../trino-cockroachdb/src/test/java/io/trino/plugin/cockroachdb/TestCockroachDbTypeMapping.java
Show resolved
Hide resolved
...in/trino-cockroachdb/src/test/java/io/trino/plugin/cockroachdb/TestingCockroachDbServer.java
Show resolved
Hide resolved
...in/trino-cockroachdb/src/test/java/io/trino/plugin/cockroachdb/TestingCockroachDbServer.java
Show resolved
Hide resolved
- based on PostgreSQL connector since it supports same JDBC driver - plugin code with tests - documentation - integration into server package
|
@mosabua Are you still working on this? |
|
Would love to but havent had time since the initial stab .. feel free to take it over |
|
I am closing this PR since I wont have time to drive this further any time soon. Anybody please feel free to take this work and build upon it to get CockroachDB connector off the ground. I will keep my branch and potentially work on it again but there is no ETA and there should be no expectation that I can drive this further. |
Filing this mostly as an fyi
Current status:
Todo: