[SPARK-44262][SQL] Add `dropTable` and `getInsertStatement` to JdbcDialect #41855

Hisoka-X · 2023-07-04T14:04:51Z

What changes were proposed in this pull request?

This PR add dropTable function to JdbcDialect. So user can override dropTable SQL by other JdbcDialect like Neo4J
Neo4J Drop case

MATCH (m:Person {name: 'Mark'})
DELETE m

Also add getInsertStatement for same reason.
Neo4J Insert case

MATCH (p:Person {name: 'Jennifer'})
SET p.birthdate = date('1980-01-01')
RETURN p

Neo4J SQL(in fact named CQL) not like normal SQL, but it have JDBC driver.

Why are the changes needed?

Make JdbcDialect more useful

Does this PR introduce any user-facing change?

No

How was this patch tested?

exist test

Hisoka-X · 2023-07-04T14:05:28Z

cc @HyukjinKwon @MaxGekk

MaxGekk

other JdbcDialect like Neo4J

@Hisoka-X What's the issue Neo4J? Could you provide more details in PR's description, please.

LGTM in general.

HyukjinKwon · 2023-07-04T23:44:39Z

cc @sadikovi FYI

Hisoka-X · 2023-07-05T02:39:38Z

@fbiville Hi, can you tell us more detail about how neo4j use drop table or insert statement in JDBC? Thanks.

Hisoka-X · 2023-07-05T02:41:01Z

other JdbcDialect like Neo4J

@Hisoka-X What's the issue Neo4J? Could you provide more details in PR's description, please.

LGTM in general.

Already Update. FYI, also move insertStatement to JdbcDialect.

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

fbiville · 2023-07-07T13:00:36Z

@fbiville Hi, can you tell us more detail about how neo4j use drop table or insert statement in JDBC? Thanks.

Hello, I experimented using https://github.com/neo4j-contrib/neo4j-jdbc and a third-party, closed-source SDK based on Spark.

The Neo4j JDBC connector supports only Cypher queries, not SQL queries.

After a conversation with the third-party maintainer, I learnt they were calling org.apache.spark.sql.execution.datasources.jdbc.JdbcUtils#getSchemaOption, which would fail right away since there is no JdbcDialect registered for Neo4j.

However, I quickly realize that supplying a Neo4j dialect would not be enough, as some SQL statements are hardcoded directly in JdbcUtils and those would not work against Neo4j.

Does that help?

Hisoka-X · 2023-08-03T15:09:57Z

Hi @MaxGekk , can you continue review this PR? Please.

Hisoka-X · 2023-09-01T07:45:35Z

Since the 3.5.0 rc3 will failed, shell we add this PR to 3.5.0 now? @cloud-fan @MaxGekk @HyukjinKwon

HyukjinKwon · 2023-09-04T00:38:07Z

Nah, let's don't add a new API to 3.5.0 at this moment.

Hisoka-X · 2023-09-04T00:50:10Z

Nah, let's don't add a new API to 3.5.0 at this moment.

Got it, let me change since to 4.0.0

cloud-fan · 2023-09-04T05:49:08Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

+   * @return The SQL query to use for insert data into table.
+   */
+  @Since("4.0.0")
+  def getInsertStatement(


let's use consistent naming in this file. How about def insertIntoTable?

~~My concerns is this method only return sql string statement, not do insert into table.~~

we already have createTable, alterTable, and you just added dropTable :)

I mean the behavior of getInsertStatement is return the string SQL, unlike createTable, alterTable,dropTable do create/alter/drop to database. The getInsertStatement dosen't insert data into table, it just used to prepare JDBCStatement, change it to insertIntoTable seem like not match with it behavior.

oh just realized that other method doesn't actually perform either. Let me change it.

Another question, createTable do create table, but other method just return SQL string, should we let createTable only return string to make JdbcDialect behave more consistently?

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala

beliefer

LGTM if tests passed.
cc @cloud-fan

cloud-fan · 2023-10-13T12:49:06Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala

      rddSchema: StructType,
      tableSchema: Option[StructType],
      isCaseSensitive: Boolean,
      dialect: JdbcDialect): String = {


note: getInsertStatement is not a public API but we don't stop people from calling JdbcUtils as there is no private[spark]. I'm fine keeping it to avoid troubles for third-party Spark vendors.

MaxGekk · 2023-10-16T08:54:44Z

+1, LGTM. Merging to master.
Thank you, @Hisoka-X and @cloud-fan @beliefer @yaooqinn @HyukjinKwon @fbiville for review.

Hisoka-X · 2023-10-16T09:19:30Z

Thanks @MaxGekk and all!

github-actions bot added the SQL label Jul 4, 2023

Hisoka-X force-pushed the SPARK-44262_JDBCUtils_improve branch from 4ed366d to 06ddb9a Compare July 4, 2023 15:01

MaxGekk reviewed Jul 4, 2023

View reviewed changes

Hisoka-X changed the title ~~[SPARK-44262][SQL] Add DropTable to JdbcDialect~~ [SPARK-44262][SQL] Add DropTable and getInsertStatement to JdbcDialect Jul 5, 2023

yaooqinn reviewed Jul 5, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Jul 5, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Show resolved Hide resolved

MaxGekk changed the title ~~[SPARK-44262][SQL] Add DropTable and getInsertStatement to JdbcDialect~~ [SPARK-44262][SQL] Add dropTable and getInsertStatement to JdbcDialect Jul 6, 2023

Hisoka-X requested a review from MaxGekk July 10, 2023 11:40

cloud-fan reviewed Sep 4, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Show resolved Hide resolved

cloud-fan reviewed Sep 4, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Show resolved Hide resolved

cloud-fan reviewed Sep 4, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

Hisoka-X requested a review from cloud-fan September 12, 2023 02:06

cloud-fan reviewed Oct 12, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Oct 12, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

beliefer reviewed Oct 12, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala Outdated Show resolved Hide resolved

Hisoka-X force-pushed the SPARK-44262_JDBCUtils_improve branch from c9f38da to 02339a3 Compare October 12, 2023 11:45

cloud-fan reviewed Oct 12, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

beliefer reviewed Oct 13, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala Outdated Show resolved Hide resolved

beliefer reviewed Oct 13, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala Outdated Show resolved Hide resolved

Hisoka-X commented Oct 13, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala Outdated Show resolved Hide resolved

update

a9dfc49

Hisoka-X force-pushed the SPARK-44262_JDBCUtils_improve branch from 4beea08 to a9dfc49 Compare October 13, 2023 12:27

beliefer approved these changes Oct 13, 2023

View reviewed changes

cloud-fan approved these changes Oct 13, 2023

View reviewed changes

cloud-fan reviewed Oct 13, 2023

View reviewed changes

MaxGekk approved these changes Oct 16, 2023

View reviewed changes

MaxGekk closed this in 6994bad Oct 16, 2023

Hisoka-X deleted the SPARK-44262_JDBCUtils_improve branch October 16, 2023 10:09

[SPARK-44262][SQL] Add dropTable and getInsertStatement to JdbcDialect #41855

[SPARK-44262][SQL] Add dropTable and getInsertStatement to JdbcDialect #41855

Uh oh!

Conversation

Hisoka-X commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Hisoka-X commented Jul 4, 2023

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jul 4, 2023

Uh oh!

Hisoka-X commented Jul 5, 2023

Uh oh!

Hisoka-X commented Jul 5, 2023

Uh oh!

Uh oh!

Uh oh!

fbiville commented Jul 7, 2023

Uh oh!

Hisoka-X commented Aug 3, 2023

Uh oh!

Hisoka-X commented Sep 1, 2023

Uh oh!

HyukjinKwon commented Sep 4, 2023

Uh oh!

Hisoka-X commented Sep 4, 2023

Uh oh!

cloud-fan Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

Hisoka-X Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

Hisoka-X Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hisoka-X Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beliefer left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Oct 16, 2023

Uh oh!

Hisoka-X commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[SPARK-44262][SQL] Add `dropTable` and `getInsertStatement` to JdbcDialect #41855

[SPARK-44262][SQL] Add `dropTable` and `getInsertStatement` to JdbcDialect #41855

Hisoka-X commented Jul 4, 2023 •

edited

Loading

Hisoka-X Sep 4, 2023 •

edited

Loading

Hisoka-X Sep 4, 2023 •

edited

Loading