Skip to content

[SPARK-26499] [SQL] JdbcUtils.makeGetter does not handle ByteType #23400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
twdsilva wants to merge 5 commits into from
Closed

[SPARK-26499] [SQL] JdbcUtils.makeGetter does not handle ByteType #23400

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
749afc3
JdbcUtils.getCatalystType maps TINYINT to IntegerType instead of Byte…
twdsilva Dec 29, 2018
c202053
Revert change in getCatalystType that maps TINYINT to ByteType. Added…
twdsilva Dec 30, 2018
ae34533
fix formatting
twdsilva Dec 30, 2018
1c487b7
Fix formatting
twdsilva Dec 31, 2018
a5dedac
Formatting change : moved ByteType after ShortType in JdbcUtils.makeG…
twdsilva Dec 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Revert change in getCatalystType that maps TINYINT to ByteType. Added… 
… test that uses custom JdbcDialect that maps TINYINT to ByteType
  • Loading branch information
@twdsilva
twdsilva committed Dec 30, 2018
commit c202053a8297872faf70c17ebe38ef49fdd913ac
2 changes: 1 addition & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ object JdbcUtils extends Logging {
case java.sql.Types.TIMESTAMP => TimestampType
case java.sql.Types.TIMESTAMP_WITH_TIMEZONE
=> null
case java.sql.Types.TINYINT => ByteType
case java.sql.Types.TINYINT => IntegerType
case java.sql.Types.VARBINARY => BinaryType
case java.sql.Types.VARCHAR => StringType
case _ =>
Expand Down
25 changes: 23 additions & 2 deletions sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import java.util.{Calendar, GregorianCalendar, Properties}

import org.h2.jdbc.JdbcSQLException
import org.scalatest.{BeforeAndAfter, PrivateMethodTester}

import org.apache.spark.SparkException
import org.apache.spark.sql.{AnalysisException, DataFrame, QueryTest, Row}
import org.apache.spark.sql.catalyst.parser.CatalystSqlParser
Expand Down Expand Up @@ -56,6 +55,17 @@ class JDBCSuite extends QueryTest
Some(StringType)
}

val testH2DialectTinyInt = new JdbcDialect {
override def canHandle(url: String): Boolean = url.startsWith("jdbc:h2")
override def getCatalystType(sqlType: Int, typeName: String, size: Int, md: MetadataBuilder) :
Option[DataType] = {
sqlType match {
case java.sql.Types.TINYINT => Some(ByteType)
Copy link
Member

@dongjoon-hyun dongjoon-hyun Nov 19, 2019

Choose a reason for hiding this comment

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

I'm wondering if this is okay in this H2 dialect context.
We made testH2DialectTinyInt like this, but theoretically, TINYINT is still unsigned and Spark ByteType is signed.

Copy link
Member

@HyukjinKwon HyukjinKwon Nov 19, 2019

Choose a reason for hiding this comment

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

Yeah, I think we found similar issue in another PR. @twdsilva can you check and fix it if needed?

case _ => None
}
}
}

before {
Utils.classForName("org.h2.Driver")
// Extra properties that will be specified for our database. We need these to test
Expand Down Expand Up @@ -572,7 +582,7 @@ class JDBCSuite extends QueryTest
assert(rows.length === 1)
assert(rows(0).getInt(0) === 1)
assert(rows(0).getBoolean(1) === false)
assert(rows(0).getByte(2) === 3)
assert(rows(0).getInt(2) === 3)
assert(rows(0).getInt(3) === 4)
assert(rows(0).getLong(4) === 1234567890123L)
}
Expand Down Expand Up @@ -693,6 +703,17 @@ class JDBCSuite extends QueryTest
JdbcDialects.unregisterDialect(testH2Dialect)
}

test("Map TINYINT to ByteType via JdbcDialects") {
JdbcDialects.registerDialect(testH2DialectTinyInt)
Copy link
Member

@HyukjinKwon HyukjinKwon Nov 19, 2019

Choose a reason for hiding this comment

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

Oh, @dongjoon-hyun, actually this test seems fine. The test dialect seems only scoped for this specific case.

Copy link
Member

@HyukjinKwon HyukjinKwon Nov 19, 2019

Choose a reason for hiding this comment

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

although it's a bit odd given #23400 (comment), at least this test is scoped and won't affect other tests about unsigned TINYINT cases.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Nov 19, 2019

Choose a reason for hiding this comment

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

Originally, I suspected this dialect is valid for H2 database's type.

Luckily, TINYINT of H2 database seems to be also -128 to 127.

In this case, I'm okay~

Copy link
Member

@HyukjinKwon HyukjinKwon Nov 19, 2019

Choose a reason for hiding this comment

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

Thank you for verifying this case @dongjoon-hyun

val df = spark.read.jdbc(urlWithUserAndPass, "test.inttypes", new Properties())
val rows = df.collect()
assert(rows.length === 2)
assert(rows(0).get(2).isInstanceOf[Byte])
assert(rows(0).getByte(2) === 3)
assert(rows(1).isNullAt(2))
JdbcDialects.unregisterDialect(testH2DialectTinyInt)
}

test("Default jdbc dialect registration") {
assert(JdbcDialects.get("jdbc:mysql://127.0.0.1/db") == MySQLDialect)
assert(JdbcDialects.get("jdbc:postgresql://127.0.0.1/db") == PostgresDialect)
Expand Down