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

Added trino-sybase connector #8488

Closed
wants to merge 16 commits into from
Closed

Added trino-sybase connector #8488

wants to merge 16 commits into from

Conversation

academy-codex
Copy link
Contributor

@academy-codex academy-codex commented Jul 7, 2021 •
edited by ebyhr
Loading

Fixes #2481.

Description: Adding a new sybase connector compatible with jTDS type 4 driver.

  • Add documentation
  • Check JDBC driver license

@cla-bot cla-bot bot added the cla-signed label Jul 7, 2021
@academy-codex academy-codex mentioned this pull request Jul 7, 2021
Closed
@academy-codex
Copy link
Contributor Author

@ebyhr Raising a fresh PR as the old fork was deleted by me a while back.
I would like to start discussion on this thread and get it to a closure.

@academy-codex
Copy link
Contributor Author

Removing aggregation pushdown from this PR.
Will raise a seperate PR for that.

@hashhar hashhar self-requested a review July 8, 2021 07:59
@ebyhr ebyhr self-requested a review July 8, 2021 08:14
ebyhr
ebyhr reviewed Jul 15, 2021
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.

Left some initial comments.

Please

  • add trino-sybase module to root pom.xml
  • add trino-sybase module to core/trino-server/src/main/provisio/presto.xml
  • add sybase.rst to docs/src/main/sphinx/connector
  • fix docs/src/main/sphinx/connector.rst

plugin/trino-sybase/src/test/resources/container-license-acceptance.txt Outdated Show resolved Hide resolved
plugin/trino-sybase/pom.xml Outdated Show resolved Hide resolved
plugin/trino-sybase/src/test/java/io/trino/plugin/sybase/SybaseQueryRunner.java Outdated Show resolved Hide resolved
plugin/trino-sybase/src/test/java/io/trino/plugin/sybase/SybaseQueryRunner.java Outdated Show resolved Hide resolved
plugin/trino-sybase/src/test/java/io/trino/plugin/sybase/SybaseQueryRunner.java Show resolved Hide resolved
plugin/trino-sybase/src/test/java/io/trino/plugin/sybase/TestSybasePlugin.java Outdated Show resolved Hide resolved
plugin/trino-sybase/src/test/java/io/trino/plugin/sybase/SybaseDataLoader.java Show resolved Hide resolved
plugin/trino-sybase/src/main/java/io/trino/plugin/sybase/SybaseClient.java Outdated
Comment on lines 59 to 60
// SqlServer supports 2100 parameters in prepared statement, let's create a space for about 4 big IN predicates
public static final int SYBASE_MAX_LIST_EXPRESSIONS = 500;
Copy link
Member

Choose a reason for hiding this comment

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

Remove if it's unrelated to Sybase connector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sybase too has similar limit for max expressions

Copy link
Member

Choose a reason for hiding this comment

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

Then, please update the above comment since it mentions "SqlServer".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed that.

Copy link
Member

@ebyhr ebyhr Jul 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No need to remove that. The comment helps us understand Sybase limitation.

plugin/trino-sybase/src/main/java/io/trino/plugin/sybase/SybaseClient.java Outdated Show resolved Hide resolved
plugin/trino-sybase/src/main/java/io/trino/plugin/sybase/SybaseClient.java
String sql = format(
"sp_rename %s, %s",
singleQuote(catalogName, schemaName, tableName),
singleQuote(newTable.getTableName()));
Copy link
Member

Choose a reason for hiding this comment

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

As far as I checked this rename logic locally, this method throws an error. Perhaps, Sybase doesn't allow passing catalog nor schema names in this procedure?
http://infocenter.sybase.com/help/index.jsp?topic=/com.sybase.infocenter.dc36273.1572/html/sprocs/X37949.htm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me check on this..

@academy-codex
Copy link
Contributor Author

Let me revise with review comments.
Thanks for the initial review.

@ebyhr ebyhr added the needs-docs This pull request requires changes to the documentation label Jul 17, 2021
@colebow
Copy link
Member

colebow commented Oct 27, 2022

👋 @academy-codex - this PR is inactive and doesn't seem to be under development. If you'd like to continue work on this at any point in the future, feel free to re-open.

@colebow colebow closed this Oct 27, 2022
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

@hashhar hashhar Awaiting requested review from hashhar

Assignees

@academy-codex academy-codex

Labels
cla-signed needs-docs This pull request requires changes to the documentation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add sybase connector
3 participants
@academy-codex @colebow @ebyhr