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.

Sign up for GitHub

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

Jump to bottom

Add Sybase connector #3462

Closed
wants to merge 18 commits into from
Closed

Add Sybase connector #3462

wants to merge 18 commits into from

Conversation

academy-codex
Copy link
Contributor

fixes #2481

Will be adding the supporting results and documentation in a couple days.

@cla-bot cla-bot bot added the cla-signed label Apr 17, 2020
@ebyhr
Copy link
Member

ebyhr commented Apr 17, 2020

Thanks for your contribution!
There is an exising PR #2976. Could you talk with @tooptoop4 ?

Also, I would suggest you join the community Slack https://prestosql.io/slack.html

tooptoop4
tooptoop4 suggested changes Apr 17, 2020
View reviewed changes
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Outdated Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Outdated Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Outdated Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Outdated Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Outdated Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Outdated Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Outdated Show resolved Hide resolved
presto-sybase/src/main/java/io.prestosql.plugin.sybase/SybaseClient.java Outdated Show resolved Hide resolved
@tooptoop4 tooptoop4 mentioned this pull request Apr 18, 2020
Closed
@tooptoop4
Copy link
Contributor

@academy-codex would you be able to add aggregate pushdown like 193872a#diff-e4b92558819e35928d8d20100000291dda1e15cf907c1d2f06ba4015b0afd6cc?

@academy-codex
Copy link
Contributor Author

@tooptoop4 We are working on it. Have updated my branch with latest 341 changes for now.

@academy-codex
Copy link
Contributor Author

@academy-codex would you be able to add aggregate pushdown like 193872a#diff-e4b92558819e35928d8d20100000291dda1e15cf907c1d2f06ba4015b0afd6cc?

Hey @tooptoop4 ,
Have added aggregation pushdown for sybase.. working on numeric columns except for INT type. Need to explicitly handle that.

@academy-codex
Copy link
Contributor Author

@findepi @ebyhr @martint Can we start conversation in this thread again. The jTDS driver works with Sybase and I have tested it as well with the aggregation pushdown. How do we go about merging this PR from here. Can you please guide me with the next items to complete to get this merged.

@findepi
Copy link
Member

findepi commented Nov 4, 2020

Ideally we should have some tests for a connector.
For a JDBC connector we typically have this

  • smoke test (see TestPostgreSqlIntegrationSmokeTest)
  • "distirbuted queries test" (see TestPostgreSqlDistributedQueries)
  • data type mapping (see TestPostgreSqlTypeMapping)

for these, you will need a sybase instance. you can follow TestingPostgreSqlServer and PostgreSqlQueryRunner examples, perhaps using https://hub.docker.com/r/datagrip/sybase with org.testcontainers.containers.GenericContainer .

To simplify review, i would recommend adding as little functionality and as much testing as possible.
For example, it's OK to defer aggregate pushdown implementation to another PR, and leave this one focused on solid connector base implementation.

@academy-codex
Copy link
Contributor Author

academy-codex commented Nov 5, 2020 •
edited
Loading

Ideally we should have some tests for a connector.
For a JDBC connector we typically have this

  • smoke test (see TestPostgreSqlIntegrationSmokeTest)
  • "distirbuted queries test" (see TestPostgreSqlDistributedQueries)
  • data type mapping (see TestPostgreSqlTypeMapping)

for these, you will need a sybase instance. you can follow TestingPostgreSqlServer and PostgreSqlQueryRunner examples, perhaps using https://hub.docker.com/r/datagrip/sybase with org.testcontainers.containers.GenericContainer .

To simplify review, i would recommend adding as little functionality and as much testing as possible.
For example, it's OK to defer aggregate pushdown implementation to another PR, and leave this one focused on solid connector base implementation.

@findepi Thanks.
Hey I tried implementing the test cases with refactoring.
How do i use the docker thing ?

How do i specify the database, username and password ? Generic container doesnt have those methods like postgre ?

ebyhr
ebyhr reviewed Nov 28, 2020
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do i specify the database, username and password ?

Left comments about those information.

presto-sybase/src/test/java/io/prestosql/plugin/sybase/TestingSybaseServer.java
{
// private static final String MEM_SQL_LICENSE = requireNonNull(System.getProperty("memsql.license"), "memsql.license is not set");

public static final String DEFAULT_TAG = "memsql/cluster-in-a-box:centos-7.1.4-516dfe4088-1.9.6-1.6.1";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static final String DEFAULT_TAG = "memsql/cluster-in-a-box:centos-7.1.4-516dfe4088-1.9.6-1.6.1";
public static final String DEFAULT_TAG = "datagrip/sybase:16.0";

presto-sybase/src/test/java/io/prestosql/plugin/sybase/TestingSybaseServer.java
@Override
public String getUsername()
{
return "root";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return "root";
return "sa";

presto-sybase/src/test/java/io/prestosql/plugin/sybase/TestingSybaseServer.java
@Override
public String getPassword()
{
return "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return "";
return "myPassword";

@findepi findepi changed the title Adding presto-sybase connector Add Sybase connector Nov 29, 2020
@findepi findepi added the enhancement New feature or request label Nov 29, 2020
@academy-codex
Copy link
Contributor Author

@ebyhr I want to get this rolling again. I have prted the code of sybase to trino and have tested against docker image.
Let me complete the test cases requirement and push a commit, then we can maybe start this thread again and close it ?

@ebyhr
Copy link
Member

ebyhr commented Jun 27, 2021

@academy-codex Yes, I would review once you rebase and push a new commit. There were some change around tests since last review, so you will need to take care of that (e.g. BaseJdbcConnectorTest was added).

@academy-codex
Copy link
Contributor Author

Continued in PR #8488.

@findepi
Copy link
Member

findepi commented Jul 12, 2021

Continued in PR #8488.

@findepi findepi closed this Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr left review comments

@tooptoop4 tooptoop4 tooptoop4 requested changes

Assignees

@academy-codex academy-codex

Labels
cla-signed enhancement New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add sybase connector
4 participants
@academy-codex @ebyhr @tooptoop4 @findepi