[SPARK-45561][SQL] Add proper conversions for TINYINT in MySQLDialect #43390

michaelzhan-db · 2023-10-16T22:27:19Z

What changes were proposed in this pull request?

Change MySql Dialect to convert catalyst TINYINT into MySQL TINYINT rather than BYTE and INTEGER. BYTE does not exist in MySQL. The same applies to MsSqlServerDialect.

Why are the changes needed?

Since BYTE type does not exist in MySQL, any casts that could be pushed down involving BYTE type would fail.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT pass.

Was this patch authored or co-authored using generative AI tooling?

No

beliefer · 2023-10-17T02:36:28Z

#43232 mapped TINYINT to ShortType for MsSqlServerDialect.
It looks very interesting. The behavior of these two PRs is somewhat opposite.

beliefer · 2023-10-17T02:41:32Z

@michaelzhan-db Could you add tests? Please refer #43232

beliefer · 2023-10-17T02:43:25Z

cc @gengliangwang @cloud-fan

yaooqinn · 2023-10-17T04:06:03Z

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

@@ -128,6 +128,7 @@ private object MsSqlServerDialect extends JdbcDialect {
    case BinaryType => Some(JdbcType("VARBINARY(MAX)", java.sql.Types.VARBINARY))
    case ShortType if !SQLConf.get.legacyMsSqlServerNumericMappingEnabled =>
      Some(JdbcType("SMALLINT", java.sql.Types.SMALLINT))
+    case ByteType => Option(JdbcType("TINYINT", java.sql.Types.TINYINT))


Is Ms SQL TINYINT sufficient for negative values of spark's ByteType?

@michaelzhan-db The range of MsSqlServer's TINYINT from 0 to 255. But Spark ByteType could represent the negative value.

My bad, I have been too hasty in creating the PR. I will update the PR and JIRA to just MySQL. Thanks for catching that!

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

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala

beliefer · 2023-10-18T02:44:07Z

...ocker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MySQLIntegrationSuite.scala

@@ -54,10 +55,10 @@ class MySQLIntegrationSuite extends DockerJDBCIntegrationSuite {
    conn.prepareStatement("INSERT INTO tbl VALUES (42,'fred')").executeUpdate()
    conn.prepareStatement("INSERT INTO tbl VALUES (17,'dave')").executeUpdate()

-    conn.prepareStatement("CREATE TABLE numbers (onebit BIT(1), tenbits BIT(10), "
+    conn.prepareStatement("CREATE TABLE numbers (onebit BIT(1), tenbits BIT(10), tiny TINYINT, "


Shall we add tiny TINYINT at last?

I wanted to maintain the rough order of increasing size of numeric type but I am ok with adding it at the end instead.

We can reduce the change if we add it at the end. It is easy to review.

beliefer · 2023-10-20T01:49:02Z

...ocker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MySQLIntegrationSuite.scala

@@ -43,7 +43,8 @@ class MySQLIntegrationSuite extends DockerJDBCIntegrationSuite {
    override val usesIpc = false
    override val jdbcPort: Int = 3306
    override def getJdbcUrl(ip: String, port: Int): String =
-      s"jdbc:mysql://$ip:$port/mysql?user=root&password=rootpass"
+      s"jdbc:mysql://$ip:$port/" +
+        s"mysql?user=root&password=rootpass&allowPublicKeyRetrieval=true&useSSL=false"


Why add allowPublicKeyRetrieval and useSSL?

beliefer · 2023-10-21T07:30:53Z

...ocker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MySQLIntegrationSuite.scala

+      s"jdbc:mysql://$ip:$port/" +
+        s"mysql?user=root&password=rootpass"


Please restore the two lines.

...ocker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MySQLIntegrationSuite.scala

…park/sql/jdbc/MySQLIntegrationSuite.scala

beliefer

LGTM.

MaxGekk · 2023-10-24T06:38:50Z

@michaelzhan-db Does Spark 3.4 suffer from the same issue?

MaxGekk · 2023-10-24T09:51:10Z

+1, LGTM. Merging to master/3.5.
Thank you, @michaelzhan-db and @yaooqinn @beliefer @cloud-fan for review.

### What changes were proposed in this pull request? Change MySql Dialect to convert catalyst TINYINT into MySQL TINYINT rather than BYTE and INTEGER. BYTE does not exist in MySQL. The same applies to MsSqlServerDialect. ### Why are the changes needed? Since BYTE type does not exist in MySQL, any casts that could be pushed down involving BYTE type would fail. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT pass. ### Was this patch authored or co-authored using generative AI tooling? No Closes #43390 from michaelzhan-db/SPARK-45561. Lead-authored-by: Michael Zhang <m.zhang@databricks.com> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 5092c89) Signed-off-by: Max Gekk <max.gekk@gmail.com>

github-actions bot added the SQL label Oct 16, 2023

michaelzhan-db force-pushed the SPARK-45561 branch from 36e6d21 to fa4a182 Compare October 17, 2023 00:10

michaelzhan-db changed the title ~~[SPARK-45561] Add proper conversions for SMALLINT and TINYINT in MySQLDialect~~ [SPARK-45561] Add proper conversions for TINYINT in MySQLDialect and MsSqlServerDialect Oct 17, 2023

Add proper conversions for SMALLINT and TINYINT in MySQLDialect

56c3816

michaelzhan-db force-pushed the SPARK-45561 branch from fa4a182 to 56c3816 Compare October 17, 2023 01:46

yaooqinn reviewed Oct 17, 2023

View reviewed changes

michaelzhan-db changed the title ~~[SPARK-45561] Add proper conversions for TINYINT in MySQLDialect and MsSqlServerDialect~~ [SPARK-45561] Add proper conversions for TINYINT in MySQLDialect Oct 17, 2023

michaelzhan-db added 2 commits October 17, 2023 13:55

Add tests

0acaa48

Fix tests and remove msSqlDialect changes

cd0d8d2

beliefer reviewed Oct 18, 2023

View reviewed changes

michaelzhan-db added 2 commits October 18, 2023 12:02

Address comments

24090e9

Refactor test

13e4af9

michaelzhan-db force-pushed the SPARK-45561 branch from 858667a to 13e4af9 Compare October 19, 2023 23:38

beliefer reviewed Oct 20, 2023

View reviewed changes

cloud-fan approved these changes Oct 20, 2023

View reviewed changes

michaelzhan-db added 2 commits October 20, 2023 10:38

Fix MySQLIntegrationSuite JDBC URL

b98391f

Fix tests

c07b4a3

beliefer reviewed Oct 21, 2023

View reviewed changes

cloud-fan reviewed Oct 23, 2023

View reviewed changes

...ocker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MySQLIntegrationSuite.scala Outdated Show resolved Hide resolved

Update connector/docker-integration-tests/src/test/scala/org/apache/s…

ae0862b

…park/sql/jdbc/MySQLIntegrationSuite.scala

cloud-fan approved these changes Oct 23, 2023

View reviewed changes

beliefer approved these changes Oct 23, 2023

View reviewed changes

MaxGekk changed the title ~~[SPARK-45561] Add proper conversions for TINYINT in MySQLDialect~~ [SPARK-45561][SQL] Add proper conversions for TINYINT in MySQLDialect Oct 23, 2023

MaxGekk approved these changes Oct 23, 2023

View reviewed changes

yaooqinn approved these changes Oct 23, 2023

View reviewed changes

MaxGekk closed this in 5092c89 Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-45561][SQL] Add proper conversions for TINYINT in MySQLDialect #43390

[SPARK-45561][SQL] Add proper conversions for TINYINT in MySQLDialect #43390

michaelzhan-db commented Oct 16, 2023 •

edited

Loading

beliefer commented Oct 17, 2023

beliefer commented Oct 17, 2023

beliefer commented Oct 17, 2023

yaooqinn Oct 17, 2023

beliefer Oct 17, 2023

michaelzhan-db Oct 17, 2023

beliefer Oct 18, 2023

michaelzhan-db Oct 18, 2023

beliefer Oct 19, 2023 •

edited

Loading

beliefer Oct 20, 2023

beliefer Oct 21, 2023

beliefer left a comment

MaxGekk commented Oct 24, 2023

MaxGekk commented Oct 24, 2023

		s"jdbc:mysql://$ip:$port/" +
		s"mysql?user=root&password=rootpass"

[SPARK-45561][SQL] Add proper conversions for TINYINT in MySQLDialect #43390

[SPARK-45561][SQL] Add proper conversions for TINYINT in MySQLDialect #43390

Conversation

michaelzhan-db commented Oct 16, 2023 • edited Loading

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?

Was this patch authored or co-authored using generative AI tooling?

beliefer commented Oct 17, 2023

beliefer commented Oct 17, 2023

beliefer commented Oct 17, 2023

yaooqinn Oct 17, 2023

Choose a reason for hiding this comment

beliefer Oct 17, 2023

Choose a reason for hiding this comment

michaelzhan-db Oct 17, 2023

Choose a reason for hiding this comment

beliefer Oct 18, 2023

Choose a reason for hiding this comment

michaelzhan-db Oct 18, 2023

Choose a reason for hiding this comment

beliefer Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

beliefer Oct 20, 2023

Choose a reason for hiding this comment

beliefer Oct 21, 2023

Choose a reason for hiding this comment

beliefer left a comment

Choose a reason for hiding this comment

MaxGekk commented Oct 24, 2023

MaxGekk commented Oct 24, 2023

michaelzhan-db commented Oct 16, 2023 •

edited

Loading

beliefer Oct 19, 2023 •

edited

Loading