[SPARK-26499] [SQL] JdbcUtils.makeGetter does not handle ByteType by twdsilva · Pull Request #23400 · apache/spark

twdsilva · 2018-12-29T00:11:10Z

…Type

What changes were proposed in this pull request?

Modifed JdbcUtils.makeGetter to handle ByteType.

How was this patch tested?

Added a new test to JDBCSuite that maps TINYINT to ByteType.

…Type

twdsilva · 2018-12-29T00:56:57Z

FYI @JoshRosen @hvanhovell

HyukjinKwon · 2018-12-29T10:03:24Z

I think we should also update migration guide.

HyukjinKwon · 2018-12-29T10:03:31Z

ok to test

HyukjinKwon · 2018-12-29T10:12:24Z

adding @maropu and @wangyum

SparkQA · 2018-12-29T13:10:00Z

Test build #100534 has finished for PR 23400 at commit 749afc3.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

twdsilva · 2018-12-30T03:11:30Z

In order to update the migration guide, would I have to create a separate PR for https://github.com/apache/spark-website ?

twdsilva · 2018-12-30T03:14:06Z

I will investigate why the PySpark unit tests are failing.

HyukjinKwon · 2018-12-30T03:19:02Z

Migration guide is located in https://github.com/apache/spark/blob/master/docs/sql-migration-guide-upgrade.md

HyukjinKwon · 2018-12-30T03:34:56Z

PySpark test failure looks orthogonal with this change.

… test that uses custom JdbcDialect that maps TINYINT to ByteType

SparkQA · 2018-12-30T04:57:39Z

Test build #100547 has finished for PR 23400 at commit c202053.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-30T05:37:11Z

Looks good. For completeness, the integration test should be a good to do.

twdsilva · 2018-12-30T07:01:49Z

@HyukjinKwon I ran the docker integration tests and they all passed

Run completed in 15 minutes, 45 seconds.
Total number of tests run: 21
Suites: completed 5, aborted 0
Tests: succeeded 21, failed 0, canceled 0, ignored 5, pending 0
All tests passed.
18/12/29 22:59:25 INFO ShutdownHookManager: Shutdown hook called
18/12/29 22:59:25 INFO ShutdownHookManager: Deleting directory /Users/tdsilva/spark-twdsilva/external/docker-integration-tests/target/tmp/spark-0e1546d2-c0ca-41f1-a063-802348dee0fb
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 16:46 min
[INFO] Finished at: 2018-12-29T22:59:25-08:00
[INFO] ------------------------------------------------------------------------

SparkQA · 2018-12-30T08:05:02Z

Test build #100548 has finished for PR 23400 at commit ae34533.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-30T08:38:10Z

retest this please

SparkQA · 2018-12-30T12:22:38Z

Test build #100552 has finished for PR 23400 at commit ae34533.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-31T04:11:15Z

Let me leave this open for few more days before getting it merged.

SparkQA · 2018-12-31T05:54:46Z

Test build #100582 has finished for PR 23400 at commit 1c487b7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-12-31T07:49:29Z

retest this please

SparkQA · 2018-12-31T08:05:02Z

Test build #100586 has finished for PR 23400 at commit 1c487b7.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-31T11:52:32Z

retest this please

SparkQA · 2018-12-31T13:38:23Z

Test build #100591 has finished for PR 23400 at commit 1c487b7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-31T14:03:25Z

retest this please

srowen

OK by me; I don't see any harm in supporting it

SparkQA · 2018-12-31T15:42:27Z

Test build #100593 has finished for PR 23400 at commit 1c487b7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

…etter

SparkQA · 2019-01-01T01:23:29Z

Test build #100599 has finished for PR 23400 at commit a5dedac.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2019-01-01T01:26:44Z

retest this please

HyukjinKwon · 2019-01-01T01:31:59Z

retest this please

SparkQA · 2019-01-01T04:27:40Z

Test build #100601 has finished for PR 23400 at commit a5dedac.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-01T04:42:48Z

Test build #4491 has finished for PR 23400 at commit a5dedac.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-01T05:21:52Z

Test build #100602 has finished for PR 23400 at commit a5dedac.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2019-01-01T06:10:37Z

Merged to master.

…Type ## What changes were proposed in this pull request? Modifed JdbcUtils.makeGetter to handle ByteType. ## How was this patch tested? Added a new test to JDBCSuite that maps ```TINYINT``` to ```ByteType```. Closesapache#23400 from twdsilva/tiny_int_support. Authored-by: Thomas D'Silva <tdsilva@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

## What changes were proposed in this pull request? This is a port of SPARK-26499 to 2.4 Modifed JdbcUtils.makeGetter to handle ByteType. ## How was this patch tested? Added a new test to JDBCSuite that maps ```TINYINT``` to ```ByteType```. Closes#23400 from twdsilva/tiny_int_support. Authored-by: Thomas D'Silva <tdsilvaapache.org> Signed-off-by: Hyukjin Kwon <gurwls223apache.org> ### Why are the changes needed? Changes are required to port pr #26301 (SPARK-29644) to 2.4 ### Does this PR introduce any user-facing change? No ### How was this patch tested? Yes, tested on 2.4 with using docker integration test for MySQL, MsSQLServer, Postgres ``sh /build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.11 -Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.MySQLIntegrationSuite ./build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.11 -Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.MsSqlServerIntegrationSuite ./build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.11 -Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.PostgresIntegrationSuite `` Closes#26531 from shivsood/pr_26499_2.4_port. Lead-authored-by: shivsood <shivsood@microsoft.com> Co-authored-by: Thomas D'Silva <tdsilva@apache.org> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…eType in JdbcUtils.makeGetter ### What changes were proposed in this pull request? This is a follow-up pr to fix the code coming from #23400; it replaces `update` with `setByte` for ByteType in `JdbcUtils.makeGetter`. ### Why are the changes needed? For better code. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing tests. Closes#26532 from maropu/SPARK-26499-FOLLOWUP. Authored-by: Takeshi Yamamuro <yamamuro@apache.org> Signed-off-by: Sean Owen <sean.owen@databricks.com>

dongjoon-hyun · 2019-11-19T02:25:31Z

+ size: Int,
+ md: MetadataBuilder): Option[DataType] = {
+ sqlType match {
+ case java.sql.Types.TINYINT => Some(ByteType)


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.

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

HyukjinKwon · 2019-11-19T03:04:15Z

 }

+ test("Map TINYINT to ByteType via JdbcDialects") {
+ JdbcDialects.registerDialect(testH2DialectTinyInt)


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

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.

Originally, I suspected this dialect is valid for H2 database's type.
Luckily, TINYINT of H2 database seems to be also -128 to 127.
https://www.h2database.com/html/datatypes.html#tinyint_type
In this case, I'm okay~

Thank you for verifying this case @dongjoon-hyun

JdbcUtils.getCatalystType maps TINYINT to IntegerType instead of Byte…
749afc3
…Type

HyukjinKwon reviewed Dec 29, 2018
View reviewed changes

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

twdsilva added 2 commits December 29, 2018 20:54

Revert change in getCatalystType that maps TINYINT to ByteType. Added…
c202053
… test that uses custom JdbcDialect that maps TINYINT to ByteType

fix formatting
ae34533

twdsilva changed the title ~~[SPARK-26499] [SQL] JdbcUtils.getCatalystType maps TINYINT to IntegerType instead of Byte…~~[SPARK-26499] [SQL] JdbcUtils.makeGetter does not handle ByteTypeDec 30, 2018

HyukjinKwon approved these changes Dec 30, 2018
View reviewed changes

HyukjinKwon reviewed Dec 30, 2018
View reviewed changes

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

Fix formatting
1c487b7

srowen approved these changes Dec 31, 2018
View reviewed changes

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

Formatting change : moved ByteType after ShortType in JdbcUtils.makeG…
a5dedac
…etter

asfgit closed this in 5f0ddd2Jan 1, 2019

shivsood mentioned this pull request Nov 14, 2019
[SPARK-26499][SQL][2.4] JdbcUtils.makeGetter does not handle ByteType #26531
Closed

maropu mentioned this pull request Nov 14, 2019
[SPARK-26499][SQL][FOLLOW-UP] Replace update with setByte for ByteType in JdbcUtils.makeGetter #26532
Closed

dongjoon-hyun mentioned this pull request Nov 19, 2019
[SPARK-29644][SQL][2.4] Corrected ShortType and ByteType mapping to SmallInt and TinyInt in JDBCUtils #26549
Closed

dongjoon-hyun reviewed Nov 19, 2019
View reviewed changes

HyukjinKwon assigned HyukjinKwon and unassigned HyukjinKwon Nov 19, 2019

HyukjinKwon reviewed Nov 19, 2019
View reviewed changes

twdsilva deleted the tiny_int_support branch November 19, 2019 05:16

Uh oh!

Conversation

twdsilva commented Dec 29, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

twdsilva commented Dec 29, 2018

Uh oh!

Uh oh!

HyukjinKwon commented Dec 29, 2018

Uh oh!

HyukjinKwon commented Dec 29, 2018

Uh oh!

HyukjinKwon commented Dec 29, 2018

Uh oh!

SparkQA commented Dec 29, 2018

Uh oh!

twdsilva commented Dec 30, 2018

Uh oh!

twdsilva commented Dec 30, 2018

Uh oh!

HyukjinKwon commented Dec 30, 2018

Uh oh!

HyukjinKwon commented Dec 30, 2018

Uh oh!

SparkQA commented Dec 30, 2018

Uh oh!

HyukjinKwon commented Dec 30, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twdsilva commented Dec 30, 2018

Uh oh!

SparkQA commented Dec 30, 2018

Uh oh!

HyukjinKwon commented Dec 30, 2018

Uh oh!

Uh oh!

SparkQA commented Dec 30, 2018

Uh oh!

HyukjinKwon commented Dec 31, 2018

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

kiszk commented Dec 31, 2018

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

HyukjinKwon commented Dec 31, 2018

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

HyukjinKwon commented Dec 31, 2018

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

HyukjinKwon commented Jan 1, 2019

Uh oh!

HyukjinKwon commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

HyukjinKwon commented Jan 1, 2019

Uh oh!

dongjoon-hyunNov 19, 2019

Choose a reason for hiding this comment

Uh oh!

HyukjinKwonNov 19, 2019

Choose a reason for hiding this comment

Uh oh!

twdsilva commented Dec 29, 2018•
edited
Loading

HyukjinKwon commented Dec 30, 2018•
edited
Loading