-
Notifications
You must be signed in to change notification settings - Fork 2.8k
added jdbc interpreter #211
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
Conversation
jdbc/pom.xml
Outdated
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.
Version of current master branch is '0.6.0-incubating-SNAPSHOT'
|
You might also want to add JDBCInterpreter into the default configuration and conf/zeppelin-site.xml.template |
|
@andrescelis, could you please elaborate on the added-value of this interpreter? |
|
a generic JDBC Interpreter with good property features may reduce the number of interpreters required in Zeppelin. Looks like Postgres, Phoenix, Hive, Ignite, & Tajo could all work with a generic JDBC driver. If custom code is needed for a JDBC driver, it could be added to a JDBC-extensions module instead of creating new top level Interpreters for every Zeppelin interface using JDBC. This may significantly reduce the number of PRs and necessary maintenance overall. |
|
@Leemoonsoo Thanks for letting me know I made the changes. @tzolov As @randerzander said, it would be nice to reduce the number of interpreters in Zeppelin and the JDBC module could be extended if necessary. |
|
@andrescelis, @randerzander i understand the benefits of unifying and abstracting common or related code but i fail to see how this jdbc interpreter helps with this? It still will be required to create a separate interpreter projects to cope different drivers specifics. |
|
@tzolov the java code to make a connection and execute a query on a jdbc backend is identical no matter what jdbc backend it is... the only differences are in the connection strings. I don't think that the connection strings would give the devs a lot of trouble because they would just need to provide the values and the connection string is made for them. |
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.
NullArgumentException check for parameter d
|
@andrescelis I would also love to see unit tests for: |
|
@tzolov let's set up a call so we can discuss the design of the interpreter. Please feel free to message me at [email protected]. I want to be able to understand your issue with the current design and work towards something that would help with what you mentioned about Dep insertion functionality. |
|
@andrescelis Can there be some test? About configuration, my guess is many people will want to use different database name with single jdbc interpreter configuration. And like @tzolov suggested, dep insertion for loading jdbc driver will help a lot. |
|
@Leemoonsoo yes I can add tests, but can you expand on what you mean by dep insertion? It is not a term I am familiar with. What specifically would you want to see? Thanks. |
|
@andrescelis , sorry for the delayed reply. |
|
@andrescelis Hi, I'm Jongyoul Lee and trying to merge jdbc interpreters for Zeppelin. I saw this PR is the second trial for doing this. The first one is contributed by @1teed but I cannot contact him/her at all. Could you tell me that you are still interested in this issue to contributed to Zeppelin? If you are available to do this, I think we should fix some codes. Basically, I think generic jdbc interpreter would help people use it but your PR only supports some of jdbc interpreters. Could you please make this PR more generally? |
|
@andrescelis Hi, I'll move to next PR. I hope you would understand this procedure. |
I added a generalized JDBC interpreter for Zeppelin. It doesn't come packaged with any drivers to avoid licensing concerns (I saw a Mysql interpreter was rejected because of this and similar issues would arise with SQL Server).
In order to use, you download your JDBC driver, go to the interpreter settings menu, create a '%jdbc' interpreter, and set your driver's name, type, and location. Right now it supports SQL Server, Mysql, and Postgresql.
Since this interpreter uses java.sql.DriverManager, it supports all JDBC drivers with the same java code. However, JDBC driver names and connection url formats vary. The only thing needed to support a new backend is to modify JDBCConnectionUrlBuilder.java, and add a connection url method to use the right format for your backend.
Tests are coming, I am currently unsure what the best testing strategy for this is, but I welcome advice.