-
Notifications
You must be signed in to change notification settings - Fork 3k
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 generic JDBC data source connector #3105
Conversation
…ource Make default target result size configurable in ExecutingStatementResource
This is critical for my use cases. Thanks to the community. Amazing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the commit logs and add tests.
return driverClass; | ||
} | ||
|
||
@Config("driver-class") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is unrelated to JDBC base connector. Please move to the generic module.
@Config("driver-class") | |
@Config("generic-jdbc.driver-class") |
presto-genericjdbc/src/main/java/io/prestosql/plugin/genericjdbc/GenericJdbcClient.java
Show resolved
Hide resolved
throw new PrestoException(DRIVER_NOT_FOUND, config.getDriverClass() + " not found"); | ||
} | ||
try { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the try
is redundant.
presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcErrorCode.java
Show resolved
Hide resolved
@tooptoop4, when I tried to build it, I ran into the following error: [ERROR] Failed to execute goal io.airlift.maven.plugins:sphinx-maven-plugin:2.1:generate (default) on project presto-docs: Failed to run the report: Sphinx report generation failed -> [Help 1] Is there any problems with the genericjdbc rst file, or files added into presto-docs folder by genericjdbc connector? |
Found the problem, it's due to the overline and underline in the rst file. |
I tested this generic JDBC in version 331 against Oracle. For basic stuffs it worked without any issues. The performance is extreme slow. For a 150 million records table, the following counts returned in 244 minutes, 10.2k/s. presto> select o_custkey, count() from jdbcoracle.tpch100g.orders where to_char(o_orderdate, 'mm/yyyy') = '03/1997' group by o_custkey having count() > 4; Query 20200325_052117_00005_7gt3u, FINISHED, 5 nodes However, the Oracle connector based on base JDBC running the same query, returned in 6 minutes 33 seconds, at 382k/s. presto> select o_custkey, count() from rdsoracle.tpch100g.orders where to_char(o_orderdate, 'mm/yyyy') = '03/1997' group by o_custkey having count() > 4; Query 20200325_051232_00003_7gt3u, FINISHED, 5 nodes Could you let me know, what is the difference between these two? From my understanding, it's using the same ojdbc8.jar from Oracle, both of the connectors extend base JDBC library. There is no customized getSplits handling in both connectors. |
@RugratsJ this relates to point 1 in the description "future enhancements might be: ability to overwrite custom configs. ie oracle fetchsize because default 10 is slow" read https://docs.oracle.com/cd/E18283_01/java.112/e16548/resltset.htm "By default, when Oracle JDBC runs a query, it retrieves a result set of 10 rows at a time from the database cursor. This is the default Oracle row fetch size value. You can change the number of rows retrieved with each trip to the database cursor by changing the row fetch size value." The Oracle Connector PR explicitly sets fetch size to 1000. Note for both connectors that even if u run count/group by - the entire 150mn rows are being retrieved to presto and the aggregation done within presto |
@tooptoop4, thank you for the quick response. I have the following questions:
|
|
@tooptoop4 , can you give me an example of doing a session properties in a JDBC connector, which can be changed in a session? For example, the fetch size? |
no example as the connector in its current form does not handle it, its a future idea |
I added the session properties of fetch size and the configurable fetch size, so a default fetch size is given in catalog configuration file plus I can change the default fetch size by session. It helped the speed. |
@eskabetxe, It's the same, since generic jdbc used the oracle JDBC library too. So underline is both ojdbc8.jar file. Speed wise, there is no difference from how Presto executing the query. |
I implemented isLimitGuaranteed function based upon the configuration value catalog properties file, supporting LIMIT or TOP, so the limit filter is working correctly in this generic JDBC, otherwise, it's always return false. Please add this function. |
@findepi, Thanks. I will file a new issue for Netezza. This generic jdbc connector is a valuable/great connector that we could try to use for lot of databases. Since it's generic, it will have lot of things to take care before it's complete and optimized. |
@tooptoop4 Sorry, just getting to testing this today. I built this genericjdbc plugin. I just TAR.GZ the presto-server-rpm/target/classes/presto-server-332-SNAPSHOT/plugin/genericjdbc directory. I assume I can drop this into the vanilla Presto. Please correct me if I am wrong. Do you have an example of a catalog file for this plugin? |
@dprophet examples in genericjdbc.rst |
@tooptoop4 Any possibility of adding
To your pom? Our JDBC driver requires it. I hate monkey patching installs. |
since this is not merged u are patching anyway:) |
For the generic connector, you’ll need to provide the JDBC JAR and any required dependencies. |
4 updated .java files in the root of https://github.com/tooptoop4/presto-1/tree/lightgenjdbc for 348-SNAPSHOT with fixes it has been successfully tested against: some recommended settings in .properties: |
@tooptoop4 Are you planning to work on this in the near future? |
@amitds1997 it works, I don't plan to add tests or address review comments |
Hmm.. so no plans to merge this into master 😄 ? @tooptoop4 |
@amitds1997 no plans from me, u can take it |
Are we expecting this connector to be released at some point or not anymore ? @tooptoop4 @amitds1997 |
I was not able to work on this. So, probably a maintainer can answer this question better whether this is going to be released anytime soon. |
You had me at sqlite... |
@tooptoop4 Can you please let me know if you are still working on it. Having jdbc connector seems to be very helpful |
@abhishekkrbaliase I'm not planning more work on this |
Closing out this PR as it's been established that it is no longer being worked on. If you'd like to continue work on this at any point in the future, feel free to re-open. |
Fixes #2910
Tested successfully on sqlite, sybase ase, oracle, impala, presto, dremio, sparksqlthrift, hiveserver2, postgres, mysql, db2, sqlserver, cockroachdb, derby, h2, hsqldb (ie hypersql), firebird
future enhancements might be: