Add JDBC datasource #1361

penghuo · 2023-02-20T23:36:20Z

Description

Add JDBC datasource support. Only support org.apache.hive.jdbc.HiveDriver.
Doc. https://github.com/penghuo/os-sql/blob/pr/spark/jdbc/docs/user/ppl/admin/jdbc.rst#ppl-supported-for-jdbc-connector

Issues Resolved

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov-commenter · 2023-02-20T23:52:44Z

Codecov Report

Merging #1361 (4c95018) into main (bc39346) will increase coverage by 0.05%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #1361      +/-   ##
============================================
+ Coverage     98.43%   98.48%   +0.05%     
- Complexity     3775     3913     +138     
============================================
  Files           343      355      +12     
  Lines          9364     9708     +344     
  Branches        599      621      +22     
============================================
+ Hits           9217     9561     +344     
  Misses          142      142              
  Partials          5        5

Flag	Coverage Δ
sql-engine	`98.48% <100.00%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ava/org/opensearch/sql/jdbc/JDBCStorageEngine.java	`100.00% <100.00%> (ø)`
...va/org/opensearch/sql/jdbc/JDBCStorageFactory.java	`100.00% <100.00%> (ø)`
...rg/opensearch/sql/jdbc/functions/JDBCFunction.java	`100.00% <100.00%> (ø)`
.../sql/jdbc/functions/JDBCTableFunctionResolver.java	`100.00% <100.00%> (ø)`
...pensearch/sql/jdbc/operator/JDBCQueryOperator.java	`100.00% <100.00%> (ø)`
...ensearch/sql/jdbc/operator/JDBCResponseHandle.java	`100.00% <100.00%> (ø)`
...sql/jdbc/operator/JDBCResultSetResponseHandle.java	`100.00% <100.00%> (ø)`
...l/jdbc/operator/JDBCUpdateCountResponseHandle.java	`100.00% <100.00%> (ø)`
...in/java/org/opensearch/sql/jdbc/parser/Option.java	`100.00% <100.00%> (ø)`
...g/opensearch/sql/jdbc/parser/PropertiesParser.java	`100.00% <100.00%> (ø)`
... and 1 more

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Peng Huo <penghuo@gmail.com>

docs/user/ppl/admin/jdbc.rst

doctest/build.gradle

docs/user/ppl/admin/jdbc.rst

jdbc/src/main/java/org/opensearch/sql/jdbc/functions/JDBCFunction.java

jdbc/src/main/java/org/opensearch/sql/jdbc/operator/JDBCResultSetResponseHandle.java

Yury-Fridlyand · 2023-03-06T23:59:00Z

jdbc/src/main/java/org/opensearch/sql/jdbc/parser/Option.java

+import org.opensearch.sql.exception.SemanticCheckException;
+
+/** Describe a single datasource configuration option. */
+@Builder


Why not to use a constructor or of static method?

core/src/test/java/org/opensearch/sql/analysis/AnalyzerTestBase.java

Yury-Fridlyand · 2023-03-07T00:05:31Z

docs/user/ppl/admin/jdbc.rst

+* ``url`` [Required].
+    * This parameters provides the URL to connect to a database instance provided endpoint.
+* ``driver`` [Required].
+    * This parameters provides the Driver to connect to a database instance provided endpoint. The only supported ``org.apache.hive.jdbc.HiveDriver``


Why only HiveDriver supported? There is no hardcoded restriction. Can I use anything else, for example, put SQLite driver into CLASSPATH and use it?

Why only HiveDriver supported? There is no hardcoded restriction.

Yes, only hive supported. Explain the possible value in doc.

Can I use anything else, for example, put SQLite driver into CLASSPATH and use it?

You can, if you put SQLLite into classpath. But it is not prebuild.

Signed-off-by: Peng Huo <penghuo@gmail.com>

Yury-Fridlyand · 2023-03-28T23:15:13Z

doctest/build.gradle

+            into "$projectDir/bin"
+        }
+    }
+    command "$projectDir/bin/${SPARK_BINARY}/bin/spark-class org.apache.spark.deploy.master.Master -h localhost -p 7077 --webui-port 8080"


Is it cross-platform?

To piggy-back on this, is it possible to run Spark in Docker?

MaxKsyunz

~~My main question is: can this data source be used with any JDBC driver?~~
~~If not, is it possible to make it clear to the users that Spark is the only supported JDBC data source?~~
I see now this was addressed in docs section.

I also had a question about if the change to security policies is required?

As a minor point, I flagged a few instance where Immutable* types were used instead of Java core types.

MaxKsyunz · 2023-03-09T19:45:35Z

plugin/src/main/plugin-metadata/plugin-security.policy

+  // hive2 jdbc required
+  permission java.io.FilePermission "*", "read, write";
+  permission java.security.SecurityPermission "*";


Can this be made configuration option?

If I understand this correctly, this will permanently broaden permissions of the sql plugin yet they are only necessary when a JDBC data source with Hive2 driver is used.

MaxKsyunz · 2023-03-30T18:39:16Z

doctest/build.gradle

+            into "$projectDir/bin"
+        }
+    }
+    command "$projectDir/bin/${SPARK_BINARY}/bin/spark-class org.apache.spark.deploy.master.Master -h localhost -p 7077 --webui-port 8080"


To piggy-back on this, is it possible to run Spark in Docker?

MaxKsyunz · 2023-03-30T18:51:32Z

jdbc/src/main/java/org/opensearch/sql/jdbc/operator/JDBCResultSetResponseHandle.java

+      case Types.TIMESTAMP:
+        return TIMESTAMP;
+
+      // we assume the result is json encoded string. refer https://docs.cloudera.com/HDPDocuments/HDP2/HDP-2.0.0.2/ds_Hive/jdbc-hs2.html,


Is this assumption safe to make for other JDBC drivers?

MaxKsyunz · 2023-03-30T18:52:09Z

jdbc/src/main/java/org/opensearch/sql/jdbc/operator/JDBCUpdateCountResponseHandle.java

+  @Override
+  public ExecutionEngine.Schema schema() {
+    return new ExecutionEngine.Schema(
+        ImmutableList.of(new ExecutionEngine.Schema.Column("result", "result", INTEGER)));


Suggested change

ImmutableList.of(new ExecutionEngine.Schema.Column("result", "result", INTEGER)));

List.of(new ExecutionEngine.Schema.Column("result", "result", INTEGER)));

MaxKsyunz · 2023-03-30T18:52:45Z

jdbc/src/main/java/org/opensearch/sql/jdbc/parser/PropertiesParser.java

+   */
+  public PropertiesParser() {
+    options =
+        new ImmutableList.Builder<Option>()


Can this be written using java.util.List?

luyuncheng · 2024-04-16T12:37:45Z

may I ask about why we close this pr? is there any continues planning for this?

nazarovkv · 2024-09-02T11:40:08Z

may I ask about why we close this pr? is there any continues planning for this?

+1

@penghuo is it possible to proceed with changes? Looks like community can loose valuable feature.

penghuo self-assigned this Feb 21, 2023

penghuo added 5 commits March 6, 2023 08:03

Resolve table function based StorageEngine provided function resolver

d521515

Signed-off-by: Peng Huo <penghuo@gmail.com>

jdbc ut passed

0e1ba39

Signed-off-by: Peng Huo <penghuo@gmail.com>

add jdbc

25efd3d

Signed-off-by: Peng Huo <penghuo@gmail.com>

add doctest

831a27f

Signed-off-by: Peng Huo <penghuo@gmail.com>

fix UT

bce2fbd

Signed-off-by: Peng Huo <penghuo@gmail.com>

penghuo force-pushed the pr/spark/jdbc branch from 915e0a0 to bce2fbd Compare March 6, 2023 17:11

upgrade thrift version

a66c380

Signed-off-by: Peng Huo <penghuo@gmail.com>

Yury-Fridlyand reviewed Mar 7, 2023

View reviewed changes

penghuo added 7 commits March 14, 2023 12:57

Merge branch 'main' into pr/spark/jdbc

227e4f1

Signed-off-by: Peng Huo <penghuo@gmail.com>

update

247de89

Signed-off-by: Peng Huo <penghuo@gmail.com>

fix depedency issue

a4faff8

Signed-off-by: Peng Huo <penghuo@gmail.com>

Merge branch 'main' into pr/spark/jdbc

c244f85

update vulnerable library

0ecb5dd

Signed-off-by: Peng Huo <penghuo@gmail.com>

update codec version

a0343ae

Signed-off-by: Peng Huo <penghuo@gmail.com>

update doc

5f2d6ba

Signed-off-by: Peng Huo <penghuo@gmail.com>

penghuo marked this pull request as ready for review March 21, 2023 02:52

penghuo requested a review from a team as a code owner March 21, 2023 02:52

penghuo added spark integration v2.7.0 labels Mar 21, 2023

penghuo added 2 commits March 20, 2023 19:58

update doc

2085f8e

Signed-off-by: Peng Huo <penghuo@gmail.com>

address comments

1a643ae

Signed-off-by: Peng Huo <penghuo@gmail.com>

penghuo requested review from anirudha, ps48, kavithacm, derek-ho, joshuali925, YANG-DB and rupal-bq as code owners March 23, 2023 01:11

penghuo requested review from mengweieric, vmmusings and MaxKsyunz as code owners March 23, 2023 01:11

fix UT

4c95018

Signed-off-by: Peng Huo <penghuo@gmail.com>

penghuo removed the v2.7.0 label Mar 24, 2023

Yury-Fridlyand approved these changes Mar 29, 2023

View reviewed changes

MaxKsyunz reviewed Mar 30, 2023

View reviewed changes

penghuo marked this pull request as draft April 10, 2023 16:48

penghuo closed this May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JDBC datasource #1361

Add JDBC datasource #1361

penghuo commented Feb 20, 2023 •

edited

Loading

codecov-commenter commented Feb 20, 2023 •

edited

Loading

Yury-Fridlyand Mar 6, 2023

Yury-Fridlyand Mar 7, 2023

penghuo Mar 23, 2023 •

edited

Loading

Yury-Fridlyand Mar 28, 2023

MaxKsyunz Mar 30, 2023

MaxKsyunz left a comment •

edited

Loading

MaxKsyunz Mar 9, 2023

MaxKsyunz Mar 30, 2023

MaxKsyunz Mar 30, 2023

MaxKsyunz Mar 30, 2023

MaxKsyunz Mar 30, 2023

luyuncheng commented Apr 16, 2024 •

edited

Loading

nazarovkv commented Sep 2, 2024

	ImmutableList.of(new ExecutionEngine.Schema.Column("result", "result", INTEGER)));
	List.of(new ExecutionEngine.Schema.Column("result", "result", INTEGER)));

Add JDBC datasource #1361

Add JDBC datasource #1361

Conversation

penghuo commented Feb 20, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Feb 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKsyunz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luyuncheng commented Apr 16, 2024 • edited Loading

nazarovkv commented Sep 2, 2024

penghuo commented Feb 20, 2023 •

edited

Loading

codecov-commenter commented Feb 20, 2023 •

edited

Loading

penghuo Mar 23, 2023 •

edited

Loading

MaxKsyunz left a comment •

edited

Loading

luyuncheng commented Apr 16, 2024 •

edited

Loading