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 Oracle connector #1959

Merged
merged 2 commits into from
May 16, 2020
Merged

Add Oracle connector #1959

merged 2 commits into from
May 16, 2020

Conversation

eskabetxe
Copy link
Member

@eskabetxe eskabetxe commented Nov 5, 2019

oracle plugin connector

oracle has release on Maven Central the Oracle JDBC drivers

https://repo1.maven.org/maven2/com/oracle/ojdbc/ojdbc8/19.3.0.0/ojdbc8-19.3.0.0.pom

Fixes #934

@cla-bot cla-bot bot added the cla-signed label Nov 5, 2019
@eskabetxe eskabetxe requested review from kokosing and findepi November 5, 2019 12:58
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.

Thanks for creating this PR. Left some initial comments.
Please fix the code style error.

[ERROR] src/main/java/io/prestosql/plugin/oracle/OracleClient.java:[26,8] (imports) UnusedImports: Unused import - io.prestosql.spi.type.TypeManager.
[ERROR] src/main/java/io/prestosql/plugin/oracle/OracleClient.java:[120] (regexp) RegexpMultiline: Blank line before closing brace
[ERROR] src/main/java/io/prestosql/plugin/oracle/OracleClient.java:[120] (regexp) RegexpMultiline: Multiple consecutive blank lines

Also, we need to add oracle.rst in presto-docs module and update connector.rst.

Please update the commit message as "Add Oracle connector" because we don't use past-tense. The detailed guideline is here https://github.com/prestosql/presto#development.

@ebyhr ebyhr changed the title added oracle plugin Add Oracle connector Nov 6, 2019
@eskabetxe
Copy link
Member Author

@ebyhr all done..
could you check?

presto-oracle/pom.xml Outdated Show resolved Hide resolved
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.

You can squash commits unless rebasing branch with upstream because GitHub provides us force-pushed diff. Also, please do rebase instead fo merge-commit to track upstream branch so that we can confirm the actual diff.

@eskabetxe
Copy link
Member Author

@ebyhr change all the comments..

rebased and squash the commits

@ebyhr
Copy link
Member

ebyhr commented Nov 7, 2019

@eskabetxe By the way, I would invite you to the community Slack. https://prestosql.io/slack.html There is #dev channel for Presto developers.

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
presto-main/etc/catalog/oracle.properties Outdated Show resolved Hide resolved
presto-oracle/pom.xml Outdated Show resolved Hide resolved
@findepi
Copy link
Member

findepi commented Nov 8, 2019

Two major bullets:

  • license for docker image used for testing (Add Oracle connector #1959 (comment))
    • Based on my memory, Oracle lets you use Oracle XE for dev/testing for no charge, but doesn't allow you to redistribute it. If this hasn't changed, we wouldn't be able such an image, unless it's provided directly by Oracle.
    • if this is not resolved, it limits our ability to test on CI, but is not a showstopper for this connector in general
  • license for odbc jar -- the pom.xml says it's being licensed under "Oracle Free Use Terms and Conditions (FUTC)"
    • my understanding this is not compatible with Apache License, but IANAL
    • we can workaround this by shipping the connector package without the driver and let users download the driver by themselves. In maven this can be achieved by marking the dependency provided and using presto-maven-plugin's allowedProvidedDependencies option to whitelist it

cc @martint

@eskabetxe
Copy link
Member Author

Hi @findepi,
about the question of licence of docker image its ok whith what @bsideup says here(#1959 (comment))?

and for the license of jar, if not compatible I can do what you say without problem, is there any example I can follow?

@RugratsJ
Copy link

RugratsJ commented Apr 2, 2020

@eskabetxe, the plugin built successfully, however, when using this Oracle plugin in presto, got the following errors:

presto> show schemas from rdsoracle;

Query 20200401_235740_00020_pdzzb, FAILED, 6 nodes
Splits: 87 total, 0 done (0.00%)
0:01 [0 rows, 0B] [0 rows/s, 0B/s]

Query 20200401_235740_00020_pdzzb failed: Unable to start the Universal Connection Pool: oracle.ucp.UniversalConnectionPoolException: Cannot get Connection from Datasource: java.sql.SQLException: ORA-01017: invalid username/password; logon denied

What do we need to set up in the Oracle catalog properties file?

@RugratsJ
Copy link

RugratsJ commented Apr 2, 2020

@eskabetxe, I guess it's because I'm trying to integrate this Oracle pool into version 331. This connection pool is however based upon version 332 snapshot, which base JDBC connectors have changed including the CredentialProvider class.

@ebyhr
Copy link
Member

ebyhr commented Apr 2, 2020

@RugratsJ I would recommend joining Community Slack so that we can talk quickly.
https://prestosql.io/slack.html

@RugratsJ
Copy link

RugratsJ commented Apr 2, 2020

@ebyhr, in which channel on https://prestosql.io/slack.html ?

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.

@eskabetxe Could you confirm the CI failure? Added code or 0c13d16 might break it. How about separating PR to introduce a connection pool?

@RugratsJ
Copy link

RugratsJ commented Apr 2, 2020

@eskabetxe, @ebyhr, I think it's because the user and password didn't get set when creating the pds object. We will need to read from the config file for user and password when creating the PDS. I added this logic in startConnectionPool function, now it's working.

@ebyhr
Copy link
Member

ebyhr commented Apr 3, 2020

@eskabetxe I couldn't select tables created by CTAS statement by default. It seems case-sensitivity affected it. While we can access it by case-insensitive-name-matching=true, can we improve this behavior?

CTAS

presto> create table oracle.tpch.test_ctas as select * from (values ('a')) as t(c1);
CREATE TABLE: 1 row

presto> select * from oracle.tpch.test_ctas;
Query 20200403_112116_00032_fwtga failed: line 1:15: Table 'oracle.tpch.test_ctas' does not exist

presto> show tables in oracle.tpch;
   Table
-----------
 test_ctas

Oracle side

SQL> SELECT owner, status, table_name FROM all_tables where table_name = 'test_ctas';

OWNER			       STATUS	TABLE_NAME
------------------------------ -------- ------------------------------
TPCH			       VALID	test_ctas

CREATE TABLE

presto> create table oracle.tpch.test5 (c1 int);
CREATE TABLE
presto> select * from oracle.tpch.test5;
 c1
----
(0 rows)

@eskabetxe eskabetxe requested a review from martint April 9, 2020 17:04
@RugratsJ
Copy link

Do we have updated Oracle connector that integrated with Presto version 332?

Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

Thanks for raising this PR!!

@eskabetxe eskabetxe requested review from Praveen2112 and ebyhr April 15, 2020 16:22
@ebyhr
Copy link
Member

ebyhr commented Apr 21, 2020

testDataMappingSmokeTest failed due to mapping between REAL and DOUBLE

SELECT id FROM presto_tmp_609132480497 WHERE rand() = 42 OR value = REAL '567.123'

presto_tmp_609132480497 has double type (≠real).

Recently, we replaced CREATE TABLE with CTAS in this test method and the change affected it. The populated value between CT and CTAS is different in this PR (567.1229858398438 and 567.123).

I suppose we need additional REAL type mapping.

@eskabetxe
Copy link
Member Author

@ebyhr fixed..

@ebyhr ebyhr merged commit f75484f into trinodb:master May 16, 2020
@ebyhr
Copy link
Member

ebyhr commented May 16, 2020

Merged, thanks for your work!

@ebyhr ebyhr added this to the 334 milestone May 16, 2020
@ebyhr ebyhr mentioned this pull request May 16, 2020
8 tasks
@eskabetxe eskabetxe deleted the oraclePlugin branch May 18, 2020 07:53
@dprophet
Copy link
Contributor

OMG. @ebyhr merged it! We have an Oracle connector. Thanks, this was desperately needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add Oracle connector