Uh oh!
There was an error while loading. Please reload this page.
[SPARK-28097][SQL] Map ByteType to SMALLINT for PostgresDialect - #24845
[SPARK-28097][SQL] Map ByteType to SMALLINT for PostgresDialect#24845sethfitz wants to merge 3 commits into
Conversation
PostgreSQL doesn't have TINYINT, which would map directly, but SMALLINTs are sufficient for uni-directional translation.
There was a problem hiding this comment.
Thank you for making a PR, @mojodna .
- In general,
PostgreSQLusers will not use unsupported types.
I'm wondering if your goal is the one you mentioned as side-effectsAggregatedDialect is now usable. Could you describe a little bit more about the use cases? - Also, please create a JIRA issue for this suggestion and use the JIRA id to the PR title. PR is valuable, but JIRA issue also becomes a history.
srowen
left a comment
There was a problem hiding this comment.
File a JIRA, yes. You've checked this works on postgres?
Uh oh!
There was an error while loading. Please reload this page.
@srowen yes, it works on Postgres. @dongjoon-hyunhttps://issues.apache.org/jira/browse/SPARK-28100 describes the underlying problem (i.e. why I can't provide a custom dialect with an alternate mapping). The actual use-case is writing spatial data from Spark (using Spark JTS) to Postgres using the JDBC sink (where I may need to map
Some time later, I realized that I can cast my bytes to shorts before handing off to the JDBC sink, so this is more a case of things not working as I'd expect them to. |
gatorsmile
left a comment
There was a problem hiding this comment.
Add a test to PostgresIntegrationSuite?
gatorsmile
commented
Jun 18, 2019
ok to test |
sethfitz
commented
Jun 18, 2019
@gatorsmile |
SparkQA
commented
Jun 19, 2019
Test build #106641 has finished for PR 24845 at commit
|
srowen
commented
Jun 24, 2019
@gatorsmile given the comment at #24845 (comment) are you OK with this change? |
gatorsmile
commented
Jun 25, 2019
srowen
commented
Jul 2, 2019
@mojodna what do you think about adding a simple additional test here to verify? |
sethfitz
commented
Jul 2, 2019
I'm swamped for the next couple weeks, but sure thing. Is there a specific test within |
maropu
commented
Jul 3, 2019
|
dongjoon-hyun
commented
Jul 12, 2019
Gentle ping, @mojodna . |
sethfitz
commented
Jul 12, 2019
Thanks @dongjoon-hyun. We just moved and are getting settled in, so sometime next week looks very likely. |
dongjoon-hyun
commented
Jul 12, 2019
Thank you, @mojodna . |
sethfitz
commented
Jul 17, 2019
Updated. Thanks @maropu for doing the hard part! |
SparkQA
commented
Jul 17, 2019
Test build #107798 has finished for PR 24845 at commit
|
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Merged to master.
Thank you, @mojodna , @srowen , @gatorsmile , @maropu !
dongjoon-hyun
commented
Jul 17, 2019
Thank you so much for your contribution, @mojodna . |
## What changes were proposed in this pull request? PostgreSQL doesn't have `TINYINT`, which would map directly, but `SMALLINT`s are sufficient for uni-directional translation. A side-effect of this fix is that `AggregatedDialect` is now usable with multiple dialects targeting `jdbc:postgresql`, as `PostgresDialect.getJDBCType` no longer throws (for which reason backporting this fix would be lovely): https://github.com/apache/spark/blob/1217996f1574f758d8cccc1c4e3846452d24b35b/sql/core/src/main/scala/org/apache/spark/sql/jdbc/AggregatedDialect.scala#L42 `dialects.flatMap` currently throws on the first attempt to get a JDBC type preventing subsequent dialects in the chain from providing an alternative. ## How was this patch tested? Unit tests. Closesapache#24845 from mojodna/postgres-byte-type-mapping. Authored-by: Seth Fitzsimmons <seth@mojodna.net> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
What changes were proposed in this pull request?
PostgreSQL doesn't have
TINYINT, which would map directly, butSMALLINTs are sufficient for uni-directional translation.A side-effect of this fix is that
AggregatedDialectis now usable with multiple dialects targetingjdbc:postgresql, asPostgresDialect.getJDBCTypeno longer throws (for which reason backporting this fix would be lovely):spark/sql/core/src/main/scala/org/apache/spark/sql/jdbc/AggregatedDialect.scala
Line 42 in 1217996
dialects.flatMapcurrently throws on the first attempt to get a JDBC type preventing subsequent dialects in the chain from providing an alternative.How was this patch tested?
Unit tests.