Skip to content

[ZEPPELIN-2245] separate precode into JDBCInterpreter#2121

Closed
tinkoff-dwh wants to merge 3 commits into
apache:masterfrom
tinkoff-dwh:ZEPPELIN-2245
Closed

[ZEPPELIN-2245] separate precode into JDBCInterpreter#2121
tinkoff-dwh wants to merge 3 commits into
apache:masterfrom
tinkoff-dwh:ZEPPELIN-2245

Conversation

@tinkoff-dwh
Copy link
Copy Markdown
Contributor

@tinkoff-dwh tinkoff-dwh commented Mar 10, 2017 •
edited
Loading

What is this PR for?

Separate precode by prefix. Added the ability to set different precode for different data sources

What type of PR is it?

Improvement

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-2245

How should this be tested?

  1. Set properties
default.password 	1
default.precode 	set search_path='test_path'
default.url 	jdbc:postgresql://localhost:5432/
default.user 	postgres
mysql.driver 	com.mysql.jdbc.Driver
mysql.password 	1
mysql.precode 	set @v=12
mysql.url 	jdbc:mysql://localhost:3306/
mysql.user 	root
  1. Run
    show search_path
  2. Run
%jdbc(mysql)
select @v

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

felixcheung
felixcheung reviewed Mar 11, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

can you clarify the goal of this in PR & JIRA description?

@tinkoff-dwh
Copy link
Copy Markdown
Contributor Author

tinkoff-dwh commented Mar 11, 2017

@felixcheung
done

@felixcheung
Copy link
Copy Markdown
Member

felixcheung commented Mar 12, 2017

link to #2078

felixcheung
felixcheung requested changes Mar 12, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

overall I think this is a good idea, one comment.

Comment thread docs/interpreter/jdbc.md
<td>default.precode</td>
<td></td>
<td>Some SQL which executes while opening connection</td>
</tr>
Copy link
Copy Markdown
Member

@felixcheung felixcheung Mar 12, 2017

Choose a reason for hiding this comment

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

could you add an example of using this?
perhaps in ## Binding JDBC interpter to notebook? Or another place you find

Copy link
Copy Markdown
Contributor Author

@tinkoff-dwh tinkoff-dwh Mar 12, 2017

Choose a reason for hiding this comment

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

added example

felixcheung
felixcheung reviewed Mar 12, 2017
View reviewed changes
Comment thread docs/interpreter/jdbc.md
%jdbc
show search_path
```
```sql
Copy link
Copy Markdown
Member

@felixcheung felixcheung Mar 12, 2017

Choose a reason for hiding this comment

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

I think you need an extra empty line after ```

Comment thread docs/interpreter/jdbc.md
<th>Value</th>
</tr>
<tr>
<td>default.driver</td>
Copy link
Copy Markdown
Member

@felixcheung felixcheung Mar 12, 2017

Choose a reason for hiding this comment

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

hmm, should it have/use default. here?

Copy link
Copy Markdown
Member

@felixcheung felixcheung Mar 12, 2017

Choose a reason for hiding this comment

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

maybe just omit them in this example?

Copy link
Copy Markdown
Contributor Author

@tinkoff-dwh tinkoff-dwh Mar 12, 2017

Choose a reason for hiding this comment

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

yes, here is set two data sources, each one with a precode

Comment thread docs/interpreter/jdbc.md
```
### Usage `precode`
You can set `precode` for each data source. Code runs once while opening the connection.

Copy link
Copy Markdown
Member

@felixcheung felixcheung Mar 12, 2017

Choose a reason for hiding this comment

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

you might want to add a line here to say for example, first setup properties to mysql...

@felixcheung
Copy link
Copy Markdown
Member

felixcheung commented Mar 14, 2017

thanks, I see the changes, it would be good if you could add more descriptions on what they are doing

@tinkoff-dwh
Copy link
Copy Markdown
Contributor Author

tinkoff-dwh commented Mar 14, 2017

@felixcheung
what kind of description, PR or changes from last commit?

@felixcheung
Copy link
Copy Markdown
Member

felixcheung commented Mar 14, 2017

To elaborate, this is what the documentation says

### Usage `precode`
<-- this is a new section
 +You can set `precode` for each data source. Code runs once while opening the connection.
<--- good, there's a description

 +##### Properties
<-- here we start a subsection

 +<table class="table-configuration">
 +  <tr>
 +    <th>Property Name</th>
 +    <th>Value</th>
 +  </tr>
 +  <tr>
 +    <td>default.driver</td>
 +    <td>org.postgresql.Driver</td>
<-- there we have a table immediately...
<-- what is default., mysql.? why do we need this properties/table?
 +  <tr>
 +    <td>mysql.url</td>
 +    <td>jdbc:mysql://localhost:3306/</td>
 +  <tr>
 +    <td>mysql.precode</td>
 +    <td>set @v=12</td>
 +  </tr>

 +</table>
 +
 +##### Usage
 +```sql
 +%jdbc(mysql)
 +select @v
<-- can we say here we get in return v, which is set in the precode

@tinkoff-dwh
Copy link
Copy Markdown
Contributor Author

tinkoff-dwh commented Mar 14, 2017

CI green https://travis-ci.org/tinkoff-dwh/zeppelin/builds/210855494

@felixcheung
Copy link
Copy Markdown
Member

felixcheung commented Mar 14, 2017

@Leemoonsoo it looks like Jenkins is timing out again while Travis is actually passing (though taking a long time)

felixcheung
felixcheung approved these changes Mar 14, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

LGTM

@felixcheung
Copy link
Copy Markdown
Member

felixcheung commented Mar 14, 2017

merging if no more comment

@asfgit asfgit closed this in cf131c8 Mar 15, 2017
@tinkoff-dwh tinkoff-dwh deleted the ZEPPELIN-2245 branch March 15, 2017 04:54
prabhjyotsingh pushed a commit to prabhjyotsingh/zeppelin that referenced this pull request Sep 1, 2017
@tinkoff-dwh @prabhjyotsingh
[ZEPPELIN-2245] separate precode into JDBCInterpreter
ec3c74e 
### What is this PR for?
Separate precode by prefix. Added the ability to set different precode for different data sources

### What type of PR is it?
Improvement

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-2245

### How should this be tested?
1. Set properties
 ```
default.password 	1
default.precode 	set search_path='test_path'
default.url 	jdbc:postgresql://localhost:5432/
default.user 	postgres
mysql.driver 	com.mysql.jdbc.Driver
mysql.password 	1
mysql.precode 	set v=12
mysql.url 	jdbc:mysql://localhost:3306/
mysql.user 	root
```
2. Run
`show search_path`
3. Run
```
%jdbc(mysql)
select v
```

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Tinkoff DWH <tinkoff.dwh@gmail.com>

Closes apache#2121 from tinkoff-dwh/ZEPPELIN-2245 and squashes the following commits:

970c064 [Tinkoff DWH] [ZEPPELIN-2245] editing documentation
a136a0e [Tinkoff DWH] [ZEPPELIN-2245] documentation for usage of  precode
f896ea8 [Tinkoff DWH] [ZEPPELIN-2245] separate precode into JDBCInterpreter
prabhjyotsingh pushed a commit to prabhjyotsingh/zeppelin that referenced this pull request Oct 23, 2017
@tinkoff-dwh @prabhjyotsingh
[ZEPPELIN-2245] separate precode into JDBCInterpreter
ea492a5 
### What is this PR for?
Separate precode by prefix. Added the ability to set different precode for different data sources

### What type of PR is it?
Improvement

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-2245

### How should this be tested?
1. Set properties
 ```
default.password 	1
default.precode 	set search_path='test_path'
default.url 	jdbc:postgresql://localhost:5432/
default.user 	postgres
mysql.driver 	com.mysql.jdbc.Driver
mysql.password 	1
mysql.precode 	set v=12
mysql.url 	jdbc:mysql://localhost:3306/
mysql.user 	root
```
2. Run
`show search_path`
3. Run
```
%jdbc(mysql)
select v
```

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Tinkoff DWH <tinkoff.dwh@gmail.com>

Closes apache#2121 from tinkoff-dwh/ZEPPELIN-2245 and squashes the following commits:

970c064 [Tinkoff DWH] [ZEPPELIN-2245] editing documentation
a136a0e [Tinkoff DWH] [ZEPPELIN-2245] documentation for usage of  precode
f896ea8 [Tinkoff DWH] [ZEPPELIN-2245] separate precode into JDBCInterpreter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixcheung felixcheung felixcheung approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tinkoff-dwh @felixcheung