Skip to content

[ZEPPELIN-3377] Passing Z variables to JDBC interpreter#2903

Closed
sanjaydasgupta wants to merge 25 commits into
apache:masterfrom
sanjaydasgupta:zeppelin-3342-jdbc
Closed

[ZEPPELIN-3377] Passing Z variables to JDBC interpreter#2903
sanjaydasgupta wants to merge 25 commits into
apache:masterfrom
sanjaydasgupta:zeppelin-3342-jdbc

Conversation

@sanjaydasgupta
Copy link
Copy Markdown
Contributor

What is this PR for?

This PR enables the interpolation of ZeppelinContext objects into the paragraph text of JDBC cells. It also introduces a new interpreter-level configuration parameter named zeppelin.jdbc.interpolation. This new parameter is false by default, and must be set to true to enable object interpolation. The default value of false guarantees backward compatibility for users who are not aware of the new feature.

The implementation takes the same approach that was followed in PR-2898.

I have also taken the liberty to correct a preexisting error in the description of the use of Dynamic Forms in the associated documentation (jdbc.md).

What type of PR is it?

[Feature]

Todos

  • - Task

What is the Jira issue?

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

How should this be tested?

CI Pass.

The code in this PR merely causes the JDBC interpreter to "opt-in" to the implementation already existing in the Interpreter base class - described in PR-2898. The unit tests necessary are already present in PR-2898

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes, documentation has been added to the file jdbc.md. I have also taken the liberty to correct a preexisting error in the description of the use of Dynamic Forms in the associated documentation.

Comment thread docs/interpreter/jdbc.md
SELECT name, country, performer
FROM demo.performers
WHERE name='{{"{{performer=Sheryl Crow|Doof|Fanfarlo|Los Paranoia"}}}}'
WHERE name='${performer=Sheryl Crow|Doof|Fanfarlo|Los Paranoia}'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is different, right, why this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a different thing. I have mentioned under What is this PR for?:

I have also taken the liberty to correct a preexisting error in the description of the use of
Dynamic Forms in the associated documentation (jdbc.md).

Since I was editing jdbc.md, I thought correcting the error would be useful for readers. The changed text works as intended under the current implementation, and is also according to the documentation for Dynamic Forms.

Please let me know if corrections should be handled in a different way.

Thanks for noticing this update.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok thanks

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

@felixcheung Please note my response to your earlier question about the unrelated documentation change.

Please let me know if there are any other concerns. Thanks.

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.

is there test we could add for this?

Comment thread docs/interpreter/jdbc.md
</table>

[Maven Repository: org.apache.tajo:tajo-jdbc](https://mvnrepository.com/artifact/org.apache.tajo/tajo-jdbc)
## Object Interpolation
Copy link
Copy Markdown
Member

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 empty line before ## or it will not be treated as header properly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added preceding blank line.

Comment thread docs/interpreter/jdbc.md Outdated
```

####In later JDBC cell:
```
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add sql - see L230

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the 'sql'

Comment thread docs/interpreter/jdbc.md Outdated

####In later JDBC cell:
```
%jdbc_interpreter_name select * from patents_list where
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

newline after %jdbc_interpreter_name for clarity & consistency with other example?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread docs/interpreter/jdbc.md Outdated
####In later JDBC cell:
```
%jdbc_interpreter_name select * from patents_list where
priority_country = '{country_code}' and filing_date like '2015-__-__'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is _ in 2015-__-__ intentional?
perhaps better if the example is 2015-%? or just pick 2015-01-01

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've preferred '2015-%' as using '2015-01-01' would retrieve a too small sample - and not be a realistic example.

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

@felixcheung I've fixed the 4 issues in the documentation file jdbc.md, thanks for the very careful review.

I have also added a unit test JDBCInterpreterInterpolationTest.java which tests the following:

  1. enabling and disabling the feature by setting "zeppelin.jdbc.interpolation", including the default value of false.
  2. normal substitution of {...} patterns
  3. escaping the pattern functionality by using {{...}}

Please also note that all the unit tests defined for this feature in the Interpreter base class do a thorough check of all possible uses and misuses of { and }. The above additional unit test is really more like an integration test to verify that the JDBC interpreter is inheriting and leveraging the base class's functionality correctly.

Thanks for your very helpful review.

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, thanks!

could you check the test failure though
https://travis-ci.org/sanjaydasgupta/zeppelin/builds/363808230

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

Apologies for missing the check style errors @felixcheung

The style issues in JDBCInterpreterInterpolationTest.java have been removed, and all the tests now pass. The other failures in the build are unrelated to this feature.

Thanks for your review.

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

Hi @felixcheung, can these changes be merged now?

@felixcheung
Copy link
Copy Markdown
Member

still have errors on this link https://travis-ci.org/sanjaydasgupta/zeppelin/builds/363808230?

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

Oops! I must have messed up somewhere!

I've corrected the style issues now @felixcheung, and the one remaining failure appears to be a build script problem unrelated to my code.

Thanks!

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

ping @felixcheung

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

Closing and reopening to trigger tests.

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

Hi @felixcheung please let me know if any additional changes or adjustments are needed.

The two failing tests are unrelated to the code in in this PR

Thanks.

@felixcheung
Copy link
Copy Markdown
Member

looks good!
merging if no more comment

@sanjaydasgupta
Copy link
Copy Markdown
Contributor Author

Hi @felixcheung is it possible to release this into 0.8.0?

Thanks

@felixcheung
Copy link
Copy Markdown
Member

I think so, @zjffdu what do you think about this in 0.8?

@asfgit asfgit closed this in e65f730 Apr 28, 2018
asfgit pushed a commit that referenced this pull request Apr 28, 2018
### What is this PR for?
This PR enables the interpolation of ZeppelinContext objects into the paragraph text of JDBC cells. It also introduces a new interpreter-level configuration parameter named _zeppelin.jdbc.interpolation_. This new parameter is _false_ by default, and must be set to _true_ to enable object interpolation. The default value of _false_ guarantees backward compatibility for users who are not aware of the new feature.

The implementation takes the same approach that was followed in [PR-2898](#2898).

I have also taken the liberty to correct a preexisting error in the description of the use of Dynamic Forms in the associated documentation (_jdbc.md_).

### What type of PR is it?
[Feature]

### Todos
* [ ] - Task

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

### How should this be tested?
CI Pass.

The code in this PR merely causes the JDBC interpreter to "opt-in" to the implementation already existing in the `Interpreter` base class - described in [PR-2898](#2898). The unit tests necessary are already present in PR-2898

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? Yes, documentation has been added to the file _jdbc.md_. I have also taken the liberty to correct a preexisting error in the description of the use of Dynamic Forms in the associated documentation.

Author: Sanjay Dasgupta <sanjay.dasgupta@gmail.com>
Author: Sanjay Dasgupta <sanjaydasgupta@users.noreply.github.com>

Closes #2903 from sanjaydasgupta/zeppelin-3342-jdbc and squashes the following commits:

9947d36 [Sanjay Dasgupta] Expanded * imports to remove check-style errors
094d3ce [Sanjay Dasgupta] Reduced indentation to remove check-style errors
07561f5 [Sanjay Dasgupta] Revisions after Felix Cheung's review #2903 (review)
df99ab0 [Sanjay Dasgupta] Revisions after Felix Cheung's review #2903 (review)
315a9ad [Sanjay Dasgupta] Corrected use of rlike in SQL statement
eb9194d [Sanjay Dasgupta] ZEPPELIN-3377 Updates Initial Load
0f49867 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into zeppelin-3342-hdfs
a19e998 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
77738aa [Sanjay Dasgupta] Changes to comply with Felix Cheung's comment at #2834 (comment) and Jeff Zhang's subsequent clarification
5f8505b [Sanjay Dasgupta] Changes due to Felix Cheung's comments at #2834 (review)
d600d86 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
cc3727f [Sanjay Dasgupta] Changes due the Jeff Zhang's comments at https://github.com/apache/zeppelin/pull/2834/files/1e2c87dd36dc091ca898baf8e9f178d6d1a5e600#r176930418
1e2c87d [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
3dd3dd8 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
a1703b8 [Sanjay Dasgupta] Changes suggested in Felix Cheung's review #2834 (review)
b7ddf6b [Sanjay Dasgupta] Implementing configuration (global enable/disable interpolation) following #2834 (comment)
5268803 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
1718e79 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
3b30ea2 [Sanjay Dasgupta] Reversing previous incorrect update
3beebce [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
f43fd99 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
a3215fc [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
ced295c [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
b461c82 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
2868825 [Sanjay Dasgupta] ZEPPELIN-1967: Initial updates

(cherry picked from commit e65f730)
Signed-off-by: Felix Cheung <felixcheung@apache.org>
@felixcheung
Copy link
Copy Markdown
Member

merged this to master & 0.8

mckartha pushed a commit to syntechdev/zeppelin that referenced this pull request Aug 9, 2018
### What is this PR for?
This PR enables the interpolation of ZeppelinContext objects into the paragraph text of JDBC cells. It also introduces a new interpreter-level configuration parameter named _zeppelin.jdbc.interpolation_. This new parameter is _false_ by default, and must be set to _true_ to enable object interpolation. The default value of _false_ guarantees backward compatibility for users who are not aware of the new feature.

The implementation takes the same approach that was followed in [PR-2898](apache#2898).

I have also taken the liberty to correct a preexisting error in the description of the use of Dynamic Forms in the associated documentation (_jdbc.md_).

### What type of PR is it?
[Feature]

### Todos
* [ ] - Task

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

### How should this be tested?
CI Pass.

The code in this PR merely causes the JDBC interpreter to "opt-in" to the implementation already existing in the `Interpreter` base class - described in [PR-2898](apache#2898). The unit tests necessary are already present in PR-2898

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? Yes, documentation has been added to the file _jdbc.md_. I have also taken the liberty to correct a preexisting error in the description of the use of Dynamic Forms in the associated documentation.

Author: Sanjay Dasgupta <sanjay.dasgupta@gmail.com>
Author: Sanjay Dasgupta <sanjaydasgupta@users.noreply.github.com>

Closes apache#2903 from sanjaydasgupta/zeppelin-3342-jdbc and squashes the following commits:

9947d36 [Sanjay Dasgupta] Expanded * imports to remove check-style errors
094d3ce [Sanjay Dasgupta] Reduced indentation to remove check-style errors
07561f5 [Sanjay Dasgupta] Revisions after Felix Cheung's review apache#2903 (review)
df99ab0 [Sanjay Dasgupta] Revisions after Felix Cheung's review apache#2903 (review)
315a9ad [Sanjay Dasgupta] Corrected use of rlike in SQL statement
eb9194d [Sanjay Dasgupta] ZEPPELIN-3377 Updates Initial Load
0f49867 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into zeppelin-3342-hdfs
a19e998 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
77738aa [Sanjay Dasgupta] Changes to comply with Felix Cheung's comment at apache#2834 (comment) and Jeff Zhang's subsequent clarification
5f8505b [Sanjay Dasgupta] Changes due to Felix Cheung's comments at apache#2834 (review)
d600d86 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
cc3727f [Sanjay Dasgupta] Changes due the Jeff Zhang's comments at https://github.com/apache/zeppelin/pull/2834/files/1e2c87dd36dc091ca898baf8e9f178d6d1a5e600#r176930418
1e2c87d [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
3dd3dd8 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
a1703b8 [Sanjay Dasgupta] Changes suggested in Felix Cheung's review apache#2834 (review)
b7ddf6b [Sanjay Dasgupta] Implementing configuration (global enable/disable interpolation) following apache#2834 (comment)
5268803 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
1718e79 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
3b30ea2 [Sanjay Dasgupta] Reversing previous incorrect update
3beebce [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
f43fd99 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
a3215fc [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
ced295c [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
b461c82 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
2868825 [Sanjay Dasgupta] ZEPPELIN-1967: Initial updates
mckartha pushed a commit to syntechdev/zeppelin that referenced this pull request Aug 9, 2018
### What is this PR for?
This PR enables the interpolation of ZeppelinContext objects into the paragraph text of JDBC cells. It also introduces a new interpreter-level configuration parameter named _zeppelin.jdbc.interpolation_. This new parameter is _false_ by default, and must be set to _true_ to enable object interpolation. The default value of _false_ guarantees backward compatibility for users who are not aware of the new feature.

The implementation takes the same approach that was followed in [PR-2898](apache#2898).

I have also taken the liberty to correct a preexisting error in the description of the use of Dynamic Forms in the associated documentation (_jdbc.md_).

### What type of PR is it?
[Feature]

### Todos
* [ ] - Task

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

### How should this be tested?
CI Pass.

The code in this PR merely causes the JDBC interpreter to "opt-in" to the implementation already existing in the `Interpreter` base class - described in [PR-2898](apache#2898). The unit tests necessary are already present in PR-2898

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? Yes, documentation has been added to the file _jdbc.md_. I have also taken the liberty to correct a preexisting error in the description of the use of Dynamic Forms in the associated documentation.

Author: Sanjay Dasgupta <sanjay.dasgupta@gmail.com>
Author: Sanjay Dasgupta <sanjaydasgupta@users.noreply.github.com>

Closes apache#2903 from sanjaydasgupta/zeppelin-3342-jdbc and squashes the following commits:

9947d36 [Sanjay Dasgupta] Expanded * imports to remove check-style errors
094d3ce [Sanjay Dasgupta] Reduced indentation to remove check-style errors
07561f5 [Sanjay Dasgupta] Revisions after Felix Cheung's review apache#2903 (review)
df99ab0 [Sanjay Dasgupta] Revisions after Felix Cheung's review apache#2903 (review)
315a9ad [Sanjay Dasgupta] Corrected use of rlike in SQL statement
eb9194d [Sanjay Dasgupta] ZEPPELIN-3377 Updates Initial Load
0f49867 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into zeppelin-3342-hdfs
a19e998 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
77738aa [Sanjay Dasgupta] Changes to comply with Felix Cheung's comment at apache#2834 (comment) and Jeff Zhang's subsequent clarification
5f8505b [Sanjay Dasgupta] Changes due to Felix Cheung's comments at apache#2834 (review)
d600d86 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
cc3727f [Sanjay Dasgupta] Changes due the Jeff Zhang's comments at https://github.com/apache/zeppelin/pull/2834/files/1e2c87dd36dc091ca898baf8e9f178d6d1a5e600#r176930418
1e2c87d [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
3dd3dd8 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
a1703b8 [Sanjay Dasgupta] Changes suggested in Felix Cheung's review apache#2834 (review)
b7ddf6b [Sanjay Dasgupta] Implementing configuration (global enable/disable interpolation) following apache#2834 (comment)
5268803 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
1718e79 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
3b30ea2 [Sanjay Dasgupta] Reversing previous incorrect update
3beebce [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
f43fd99 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
a3215fc [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
ced295c [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
b461c82 [Sanjay Dasgupta] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1967
2868825 [Sanjay Dasgupta] ZEPPELIN-1967: Initial updates

(cherry picked from commit e65f730)
Signed-off-by: Felix Cheung <felixcheung@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants