JDBC generic interpreter #361

vgmartinez · 2015-10-23T12:58:22Z

Only you need to add to the classpath the jdbc connector jar and the interpreter add the particular properties for each db.
In the file zeppelin-daemon.sh add:
ZEPPELIN_CLASSPATH+=":${ZEPPELIN_HOME}/jdbc/jdbc/connector jar"

felixcheung · 2015-10-23T18:49:37Z

@tzolov

…into jdbc_interpreter

jongyoul · 2015-10-25T12:16:59Z

jdbc/pom.xml

@vgmartinez I think this dependency doesn't need any longer. Could you please remove this?

jongyoul · 2015-10-25T12:26:46Z

@vgmartinez https://github.com/vgmartinez/incubator-zeppelin/blob/jdbc_interpreter/jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java#L146 How about connecting without user or password? It's better use getConnection(String url, Properties info)
https://docs.oracle.com/javase/8/docs/api/java/sql/DriverManager.html#getConnection-java.lang.String-java.util.Properties- ?

jongyoul · 2015-10-25T12:29:28Z

@vgmartinez Thanks for contribution on this issue which @Leemoonsoo and I talk about a few days ago. It will covers several Interpreters using JDBC.

jongyoul · 2015-10-25T12:41:28Z

@vgmartinez Finally, How about adding a ZEPPELIN_INTP_CLASSPATH to zeppelin-env.sh and making it added by interpreter.sh?

vgmartinez · 2015-10-25T15:09:12Z

Hi @jongyoul,
Thanks for the feedback...I will implement the suggestion....

felixcheung · 2015-10-25T23:21:12Z

There's another earlier attempt for this #211, #60
Other JDBC-like use cases perhaps you could also factor into your design? #73, #110

jongyoul · 2015-10-26T04:34:22Z

@felixcheung There are several implementations for jdbc. We should talk about it for focusing one issue. Do you have any idea?

@Leemoonsoo

Leemoonsoo · 2015-10-28T10:30:23Z

As @felixcheung mentioned, there were couple of attempt.
This patch renames existing postgresql to jdbc. So if renaming does not bring big problem, I think it can be a basement and starting point to do all the things discussed in previous attempt.

What do you think? will rename postgresql to jdbc make some problem?

felixcheung · 2015-10-28T22:05:50Z

If I recall there were 3 areas of discussions around JDBC:

JDBC url, authentication etc would need to be extensible since they vary greatly between platforms
Autocomplete hint etc would also need to be extensible since reserve words etc are different
More importantly - multi-instances - if we are to have one JDBC interpreter we need to have a way of supporting multiple instances of the interpreter? Otherwise we are limiting to work with one JDBC host at a time and can't talk to Hive, PostgresQL, Phoenix etc at the same time.

jongyoul · 2015-11-13T04:36:47Z

@felixcheung

http://docs.oracle.com/javase/7/docs/api/java/sql/DriverManager.html#getConnection(java.lang.String,%20java.util.Properties) would be a good candidate
We need to talk about it. Can Zeppelin show the hint from reading a specific setting from Interpreter tab?
I'm agree for supporting multiple instances of same Interpreters. Do you have any idea about it? Basically, generic JDBC driver will replace all JDBC drivers if we can use multiple instance.

jiekechoo · 2015-11-27T01:59:44Z

How to get this function?

jongyoul · 2015-11-27T02:04:27Z

@jiekechoo You can find a document on
http://zeppelin.incubator.apache.org/docs/0.5.5-incubating/interpreter/postgresql.html

Set the driver name as you want to postgresql.driver.name.
Set other properties
Save

jiekechoo · 2015-11-27T13:04:38Z

@jongyoul I mean how to use jdbc interpreter.

jongyoul · 2015-11-28T11:48:02Z

@jiekechoo This interpreter is based on jdbc connecttion, and this PR makes PostgreSqlInterpreter change JdbcInterpreter only. Thus you can use PostgrsSqlInterpreter to use Jdbc. @vgmartinez Could you please add docs for this interpreter? If you don't, I'll merge it first add docs later.

vgmartinez · 2015-12-11T08:06:46Z

Hi @jongyoul,
Sorry for my delay ...;) as we move forward with the discussion about the interpreter jdbc ...? I can improve this withdrawal request with previous recommendations, but I still see a problem with one of the points commented by @felixcheung , for example, to have multiple instances of the same artist ... How can we do ...?

jongyoul · 2015-12-15T02:06:21Z

@vgmartinez Sorry for delay. I've committed #455 #517 for supporting multiple instances - connections - Because that's based on this interpreter, it would be easy to fit those features in this interpreter. Could you please check those PRs? And I also hope you contribute the documentations.

jongyoul · 2015-12-15T02:09:52Z

And could you please check https://www.mail-archive.com/[email protected]/msg05504.html at dev@. I, strongly, think every contribution should be tracked.

vrajat · 2015-12-30T11:44:31Z

FWIW - I vote for this implementation of JDBC Interpreter. I understand that your focus is on well-known databases - mysql, postgres etc.
I am working with a relatively unknown JDBC driver: https://github.com/qubole/quark. This design allows me to use zeppelin with unknown or custom drivers too. I just have to put the jar in the classpath (which is a bit confusing). I have picked up this pull request and updated it to work with master of zeppelin in this branch: https://github.com/vrajat/incubator-zeppelin/tree/JDBCInterpreter.

Please feel free to use my branch if you choose this implementation and give credit to @vgmartinez . He did all the hard work.

jongyoul · 2015-12-30T14:05:23Z

@vrajat Thanks for taking an interest in this issue and being willing to contribute your code. I have a plan to merge several jdbc-like interpreters until 0.6.0-incubating. For that, I'm trying to find PRs related JDBC and contacting contributors to track their contribution. Now, I'm trying to contact the contributor of #211 and this PR is next candidate if I'm not contacting that contributor. And I also think this implementation looks the best, but I think the former contribution should be tracked and accepted at first if that contributor is still willing to give his/her time to contribute. I hope you understand my thoughts and plans. Finally, I will merge additional features of #455 and #517 into jdbc implementation. Please wait a moment and Thank you again for concerning about this issue.

jongyoul · 2015-12-30T14:17:10Z

@vrajat And I also think JDBC interpreter supports all kind of JDBC drivers.

vrajat · 2015-12-30T15:37:49Z

I read your mail. I understand why you are following this process.
I am voting for a requirement that seems to be missing in others but is
satisfied in this one. Just want to make sure you hear all requests.
Thanks for shepherding the JDBC feature. Will wait for it in the main
branch. I'll help in anyway possible.
On Wed, 30 Dec 2015 at 19:47, Jongyoul Lee [email protected] wrote:

@vrajat https://github.com/vrajat And I also think JDBC interpreter
supports all kind of JDBC drivers.

—
Reply to this email directly or view it on GitHub
#361 (comment)
.

jongyoul · 2016-01-17T16:44:19Z

@vgmartinez Sorry for late reply. @Leemoonsoo Do you introduce or use the postgresql interpreter from master branch on the upcoming meetup at 21st Jan? If you do, I'll merge this PR after that. Unless you do, I'll merge it right now.

Leemoonsoo · 2016-01-17T17:09:07Z

@jongyoul Thanks for consideration. Please feel free to merge regardless of upcoming meetup. If i need, i can always use specific commit from git repository.

jongyoul · 2016-01-17T17:11:05Z

@Leemoonsoo Thanks for the reply. I'll merge this PR after resolving conflict.

jongyoul · 2016-01-17T17:30:01Z

@vgmartinez Hi, Could you please rebase this PR one more? Some logging codes are added by another PR. After that rebase, I'll merge it immediately.

vgmartinez · 2016-01-17T18:13:27Z

hi @jongyoul,
It ready...in other PR add docs for the interpreter....

jongyoul · 2016-01-18T03:20:36Z

@vgmartinez Thanks, I'll merge it. Did you make a PR for docs? or will you make it?

tzolov · 2016-01-18T10:31:43Z

@vgmartinez , @felixcheung , @asfgit, @Leemoonsoo
Hello guys, I am sorry to jump a bit late.
First let me say that I am glad that you have decided to base the generic JDBC interpreter on the work i've done on the PostgreSqlInterpter.
I believe though that the generic interpreter should extend rather than degradating the existing functionality! For example the new generic JDBC intr. hasn't included the SqlCompleter (not sure why since the later is based on JDBC metdata functionality?) and in the same time the last commit you has removed the original PostgreSql Interpreter? (Actually one single class is still left in. Looks like a slappy commit?)

IMHO This is unacceptable functionality degradation! I would propose to return the PostgreSQL interpreter until JDBC matures? I will try to prepare a hot-fix asap.

vgmartinez · 2016-01-18T10:49:27Z

Hi @tzolov,
Yes, the interpreter is based on the PostgreSQL was mentioned #608. I think is that we should work on this interpreter and add auto-completion functionality already available in Postgres and to all others that will be added. what do you think...?

tzolov · 2016-01-18T11:43:30Z

Thanks for the quick response @vgmartinez ,

The SqlCompleter (removed by the last commit) is meant to be 100% JDBC API compliant and hopefully easy to reuse in a generic jdbc interpreter.

IMO though it is unacceptable to remove a working interpreters until the JDBC interpreter is completed. Some Zeppelin users (like me) use the master to build Zeppelin and now suddenly some essential (for us) functionality is gone.
I think that as a percussion it has to be confirmed that the new JDBC driver can support an existing (jdbc based) interpreter before the later is removed.

I suggest returning the PSQL interpreter code ASAP. Let me know if you need help.

In the meantime we can continue working on the generic JDBC. I will help porting the Completer functionality and missing tests.

jongyoul · 2016-01-18T13:05:14Z

@tzolov Hi, your opinion makes sense. @Leemoonsoo also worried about changing Postgresql to Jdbc, but I don't think it's a big problem because I will add auto-completion feature before we release Zeppelin 0.6.0. But I agree that we should also consider those who use Zeppelin from master. I propose the way to support Postgresql and it's completer is to make another PR asap. But we should make sure that this is temporary until we completed JDBC interpreter. I'll restore Postgresql interpreter soon. And personally, sorry for confusing you.

jongyoul · 2016-01-18T13:12:36Z

@vgmartinez Our goal is to move to the generic jdbc interpreter, finally. Could you please contribute a document?

beeva-victorgarcia · 2016-01-18T13:18:41Z

ok, no problem when I can add docs.

@jongyoul you move the generic jdbc and restore the PostgresQL Interprete?

jongyoul · 2016-01-18T13:21:05Z

@beeva-victorgarcia I keep this PR merged and add Postgresql interpreter again. I think this is more easier way to add Postgresql interpreter.

beeva-victorgarcia · 2016-01-18T13:22:57Z

ok...make sense

jongyoul · 2016-01-18T13:26:22Z

@tzolov Oops. I've found what the problem is. I just delete sqlCompleter only. It's my mistake. I'll fix asap.

woonsan · 2016-01-18T13:33:09Z

Big applause to you guys!
+1 on moving to jdbc interpreter which may also allow other db specific interpreter extensions in the future.
Cheers!

tzolov · 2016-01-18T13:35:53Z

@jongyoul,
Imo the safest and cleanest solution is be to revert the last (jdbc) commit and to do the changes right before committing the again.
Then it might be better to create the jdbc interpreter in parallel, and gradually replace the other jdbc-based interpreters once you can prove you don't break existing functionality.
What do you think?

- Related apache#361

tzolov · 2016-01-18T13:41:00Z

Same principles that apply for extending/evolving APIs (or Interfaces) should apply here. One shouldn't drop a new incomplete and undocumented API while removing the existing one.

jongyoul · 2016-01-18T13:45:50Z

@tzolov Yes, It would be safe in general case, but in this case, because we drop postgresql interpreter only, I think it's ok that I just add postgresql interpreter. What do you think of it? And I agree what you told is a general rule.

- Related apache#361

### What is this PR for? Restore postgresql functionality and fix some mistake #361. ### What type of PR is it? Hot Fix ### Is there a relevant Jira issue? ZEPPELIN-614 ### How should this be tested? Outline the steps to test the PR here. ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: Jongyoul Lee <[email protected]> Closes #653 from jongyoul/hotfix/restore-psql and squashes the following commits: 8917ada [Jongyoul Lee] [HOTFIX] Temporary support on Postgresql - Related #361 fde5a2b [Jongyoul Lee] [HOTFIX] Temporary support on Postgresql - Related #361

jongyoul · 2016-01-18T14:25:43Z

I fixed my mistakes. Feel free to talk to me if there's some problems. Thanks everyone for taking care of it.

beeva-victorgarcia · 2016-01-18T14:36:21Z

In the next PR I add docs and auto-completion function...sorry for this confusion

wfhartford · 2016-05-12T15:13:40Z

jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

+        connection = DriverManager.getConnection(url, user, password);
+      } else {
+        connection = DriverManager.getConnection(url, properties);
+      }


I'm curious about this if/else. Is there a specific issue this is working around? JDBC drivers interpret the user and password properties as the username and password for the connection, thus this condition seems unnecessary. What's more, it prevents passing of arbitrary additional properties along with the connection. Many JDBC drivers can be configured a great deal by adding vendor specific properties when the connection is created. This condition makes that impossible as long as a username and password are provided.

@whisperstream Hi, sorry for late reply. I agree with you and think it's my mistake and we have to fix that case that you told the above. Do you mind sharing your time to fix it?

Closes #6 (fixed by #1008) Closes #89 (fixed by #1008) Closes #46 (fixed by #140) Closes #60 (fixed by #361) Closes #211 (fixed by #361) Closes #100 (fixed by #114) Closes #190 (fixed by #796) Closes #527

vgmartinez added 4 commits October 23, 2015 14:37

generic interpreter for jdbc

53d0a81

change psql to jdbc

4f28f5a

deleted cassandra/.cache-main from commit

710699c

add descriptions

a66a5b7

vgmartinez added 2 commits October 23, 2015 22:05

Merge branch 'master' of https://github.com/apache/incubator-zeppelin …

462c3b1

…into jdbc_interpreter

fix test

09006f1

jongyoul reviewed Oct 25, 2015
View reviewed changes

jongyoul mentioned this pull request Oct 25, 2015

MapR 4 Profile corrected, Added Apache Drill Interpreter #110

Closed

jongyoul mentioned this pull request Nov 13, 2015

Presto Interpreter #68

Closed

vgmartinez added 2 commits January 17, 2016 18:48

merged with master

8046692

Merge branch 'master' into jdbc_interpreter

2513c1b

asfgit closed this in 404846f Jan 18, 2016

jongyoul added a commit to jongyoul/zeppelin that referenced this pull request Jan 18, 2016

[HOTFIX] Temporary support on Postgresql

fde5a2b

- Related apache#361

jongyoul added a commit to jongyoul/zeppelin that referenced this pull request Jan 18, 2016

[HOTFIX] Temporary support on Postgresql

8917ada

- Related apache#361

jongyoul mentioned this pull request Jan 18, 2016

[ZEPPELIN-615] Restore postgresql functionality #653

Closed

wfhartford reviewed May 12, 2016
View reviewed changes

JDBC generic interpreter #361

JDBC generic interpreter #361

Uh oh!

Conversation

vgmartinez commented Oct 23, 2015

Uh oh!

felixcheung commented Oct 23, 2015

Uh oh!

jongyoul Oct 25, 2015

Choose a reason for hiding this comment

Uh oh!

jongyoul Oct 25, 2015

Choose a reason for hiding this comment

Uh oh!

jongyoul commented Oct 25, 2015

Uh oh!

jongyoul commented Oct 25, 2015

Uh oh!

jongyoul commented Oct 25, 2015

Uh oh!

vgmartinez commented Oct 25, 2015

Uh oh!

felixcheung commented Oct 25, 2015

Uh oh!

jongyoul commented Oct 26, 2015

Uh oh!

Leemoonsoo commented Oct 28, 2015

Uh oh!

felixcheung commented Oct 28, 2015

Uh oh!

jongyoul commented Nov 13, 2015

Uh oh!

jiekechoo commented Nov 27, 2015

Uh oh!

jongyoul commented Nov 27, 2015

Uh oh!

jiekechoo commented Nov 27, 2015

Uh oh!

jongyoul commented Nov 28, 2015

Uh oh!

vgmartinez commented Dec 11, 2015

Uh oh!

jongyoul commented Dec 15, 2015

Uh oh!

jongyoul commented Dec 15, 2015

Uh oh!

vrajat commented Dec 30, 2015

Uh oh!

jongyoul commented Dec 30, 2015

Uh oh!

jongyoul commented Dec 30, 2015

Uh oh!

vrajat commented Dec 30, 2015

Uh oh!

jongyoul commented Jan 17, 2016

Uh oh!

Leemoonsoo commented Jan 17, 2016

Uh oh!

jongyoul commented Jan 17, 2016

Uh oh!

jongyoul commented Jan 17, 2016

Uh oh!

vgmartinez commented Jan 17, 2016

Uh oh!

jongyoul commented Jan 18, 2016

Uh oh!

tzolov commented Jan 18, 2016

Uh oh!

vgmartinez commented Jan 18, 2016

Uh oh!

tzolov commented Jan 18, 2016

Uh oh!

jongyoul commented Jan 18, 2016

Uh oh!

jongyoul commented Jan 18, 2016

Uh oh!

beeva-victorgarcia commented Jan 18, 2016

Uh oh!

jongyoul commented Jan 18, 2016

Uh oh!