[SPARK-32576][SQL] Support PostgreSQL `bpchar` type and array of char type #29192

kujon · 2020-07-22T16:29:44Z

What changes were proposed in this pull request?

This PR fixes the support for char(n)[], character(n)[] data types. Prior to this change, a user would get Unsupported type ARRAY exception when attempting to interact with the table with such types.

The description is a bit more detailed in the JIRA itself, but the crux of the issue is that postgres driver names char and character types as bpchar. The relevant driver code can be found here. char is very likely to be still needed, as it seems that pg makes a distinction between char(1) and char(n > 1) as per this code.

Why are the changes needed?

For completeness of the pg dialect support.

Does this PR introduce any user-facing change?

Yes, successful reads of tables with bpchar array instead of errors after this fix.

How was this patch tested?

Unit tests

…er arrays

maropu · 2020-07-23T01:32:20Z

Could you add tests in PostgresIntegrationSuite then check if the test can pass on your local env? (Note: our testing framework, Jenkins, does not run it).

maropu · 2020-07-23T01:35:45Z

bpchar is internally used in postgresql [1][2], but users can use it in DDL;

postgres=# create table t (v bpchar);
CREATE TABLE

So, I think its okay to add it for better interoperability.

maropu · 2020-07-23T02:00:34Z

btw, thanks for your first contribution, @kujon !

maropu · 2020-07-23T02:31:35Z

ok to test

kujon · 2020-07-23T05:03:37Z

Could you add tests in PostgresIntegrationSuite then check if the test can pass on your local env? (Note: our testing framework, Jenkins, does not run it).

will do!

SparkQA · 2020-07-23T07:05:02Z

Test build #126374 has finished for PR 29192 at commit 1000377.

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

dongjoon-hyun

Thank you for your first contribution, @kujon . Also, thank you for trying to add a test into PostgresIntegrationSuite.
Could you revise the PR description to be complete without referencing JIRA? The PR description will become a permanent commit log and will be read much more times.

maropu · 2020-07-28T00:02:50Z

@kujon Any update? If you get stuck, please let me know.

kujon · 2020-07-28T08:23:13Z

Hi @maropu, I just need a few more days. I'm the meantime, I struggled finding said file, do you want me to add a fresh integration test?

maropu · 2020-07-28T08:36:12Z

This one: https://github.com/apache/spark/blob/master/external/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/PostgresIntegrationSuite.scala
You can add test cases there, then run the tests via ./build/mvn -Pdocker-integration-tests -Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.PostgresIntegrationSuite

maropu · 2020-08-03T02:37:11Z

kindly ping.

dongjoon-hyun · 2020-08-09T19:46:45Z

It seems that it's too hard requirement for a new contributor. How do you want to proceed this, @maropu ?

…tgresIntegrationSuite ### What changes were proposed in this pull request? This PR intends to add tests to check if all the character types in PostgreSQL supported. The document for character types in PostgreSQL: https://www.postgresql.org/docs/current/datatype-character.html Closes #29192. ### Why are the changes needed? For better test coverage. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Add tests. Closes #29394 from maropu/pr29192. Lead-authored-by: Takeshi Yamamuro <[email protected]> Co-authored-by: kujon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit b2c45f7) Signed-off-by: Dongjoon Hyun <[email protected]>

dongjoon-hyun · 2020-08-10T01:47:47Z

I reopened this again, @kujon .

Hi, @maropu . It seems that this PR aims to support CREATE TABLE t (a char(64)[]); instead CREATE TABLE t (a bpchar).

dongjoon-hyun · 2020-08-10T01:52:29Z

ok to test

dongjoon-hyun

I manually verified that this achieved the goal. We will add a test coverage later as a follow-up.

postgres=# \d t
                     Table "public.t"
 Column |      Type       | Collation | Nullable | Default
--------+-----------------+-----------+----------+---------
 a      | character(64)[] |           |          |

scala> spark.read.jdbc("jdbc:postgresql://127.0.0.1:5432/?user=postgres&password=rootpass", "t", new java.util.Properties).show
+--------------------+
|                   a|
+--------------------+
|[str1            ...|
+--------------------+


scala> spark.read.jdbc("jdbc:postgresql://127.0.0.1:5432/?user=postgres&password=rootpass", "t", new java.util.Properties).printSchema
root
 |-- a: array (nullable = true)
 |    |-- element: string (containsNull = true)

… type ### What changes were proposed in this pull request? This PR fixes the support for char(n)[], character(n)[] data types. Prior to this change, a user would get `Unsupported type ARRAY` exception when attempting to interact with the table with such types. The description is a bit more detailed in the [JIRA](https://issues.apache.org/jira/browse/SPARK-32393) itself, but the crux of the issue is that postgres driver names char and character types as `bpchar`. The relevant driver code can be found [here](https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/jdbc/TypeInfoCache.java#L85-L87). `char` is very likely to be still needed, as it seems that pg makes a distinction between `char(1)` and `char(n > 1)` as per [this code](https://github.com/pgjdbc/pgjdbc/blob/b7fd9f3cef734b4c219e2f6bc6c19acf68b2990b/pgjdbc/src/main/java/org/postgresql/core/Oid.java#L64). ### Why are the changes needed? For completeness of the pg dialect support. ### Does this PR introduce _any_ user-facing change? Yes, successful reads of tables with bpchar array instead of errors after this fix. ### How was this patch tested? Unit tests Closes #29192 from kujon/fix_postgres_bpchar_array_support. Authored-by: kujon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 0ae94ad) Signed-off-by: Dongjoon Hyun <[email protected]>

dongjoon-hyun · 2020-08-10T02:06:18Z

Thank you for your first contribution, @kujon .
We added you to the Apache Spark contributor group and assign SPARK-32576 to you.
Welcome!

SparkQA · 2020-08-10T07:05:02Z

Test build #127252 has finished for PR 29192 at commit 1000377.

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

kujon · 2020-08-10T09:15:46Z

@dongjoon-hyun apologies, I am quite short on time recently. Thanks for accepting the PR. I'll add the tests when I find some spare time.

kujon · 2020-08-10T09:53:03Z

Ahh, I see it's being tackled in #29397 already. Thank you for that!

maropu · 2020-08-10T10:04:40Z

Never mind, @kujon! Thanks for your contribution!

…ray types in PostgresIntegrationSuite ### What changes were proposed in this pull request? This is a follow-up PR of #29192 that adds integration tests for character arrays in `PostgresIntegrationSuite`. ### Why are the changes needed? For better test coverage. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Add tests. Closes #29397 from maropu/SPARK-32576-FOLLOWUP. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>

…ray types in PostgresIntegrationSuite ### What changes were proposed in this pull request? This is a follow-up PR of #29192 that adds integration tests for character arrays in `PostgresIntegrationSuite`. ### Why are the changes needed? For better test coverage. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Add tests. Closes #29397 from maropu/SPARK-32576-FOLLOWUP. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]> (cherry picked from commit 7990ea1) Signed-off-by: Takeshi Yamamuro <[email protected]>

dongjoon-hyun · 2020-08-10T13:32:51Z

Yep. Thank you, @kujon and @maropu . We are collaborating! :)

kujon added 2 commits July 22, 2020 17:19

Added a failing unit test

59bf164

fix: postgresql: added support for blank-padded, fixed length charact…

1000377

…er arrays

probot-autolabeler bot added the SQL label Jul 22, 2020

dongjoon-hyun requested changes Jul 24, 2020

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-32393][SQL] Fix postgres bpchar array support~~ [SPARK-32393][SQL] Support PostgreSQL bpchar array Jul 24, 2020

maropu mentioned this pull request Aug 10, 2020

[SPARK-32393][SQL][TEST] Add tests for all the character types in PostgresIntegrationSuite #29394

Closed

dongjoon-hyun closed this in b2c45f7 Aug 10, 2020

dongjoon-hyun reopened this Aug 10, 2020

dongjoon-hyun changed the title ~~[SPARK-32393][SQL] Support PostgreSQL bpchar array~~ [SPARK-32576][SQL] Support PostgreSQL bpchar array Aug 10, 2020

dongjoon-hyun changed the title ~~[SPARK-32576][SQL] Support PostgreSQL bpchar array~~ [SPARK-32576][SQL] Support PostgreSQL bpchar type and array of char type Aug 10, 2020

dongjoon-hyun reviewed Aug 10, 2020

View reviewed changes

dongjoon-hyun closed this in 0ae94ad Aug 10, 2020

maropu mentioned this pull request Aug 10, 2020

[SPARK-32576][SQL][TEST][FOLLOWUP] Add tests for all the character array types in PostgresIntegrationSuite #29397

Closed

[SPARK-32576][SQL] Support PostgreSQL bpchar type and array of char type #29192

[SPARK-32576][SQL] Support PostgreSQL bpchar type and array of char type #29192

Uh oh!

Conversation

kujon commented Jul 22, 2020 • 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!

maropu commented Jul 23, 2020

Uh oh!

maropu commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Jul 23, 2020

Uh oh!

kujon commented Jul 23, 2020

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

dongjoon-hyun left a comment • 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!

maropu commented Jul 28, 2020

Uh oh!

kujon commented Jul 28, 2020

Uh oh!

maropu commented Jul 28, 2020

Uh oh!

maropu commented Aug 3, 2020

Uh oh!

dongjoon-hyun commented Aug 9, 2020

Uh oh!

dongjoon-hyun commented Aug 10, 2020

Uh oh!

dongjoon-hyun commented Aug 10, 2020

Uh oh!

dongjoon-hyun left a comment • 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!

dongjoon-hyun commented Aug 10, 2020

Uh oh!

SparkQA commented Aug 10, 2020

Uh oh!

kujon commented Aug 10, 2020

Uh oh!

kujon commented Aug 10, 2020

Uh oh!

maropu commented Aug 10, 2020

Uh oh!

dongjoon-hyun commented Aug 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[SPARK-32576][SQL] Support PostgreSQL `bpchar` type and array of char type #29192

[SPARK-32576][SQL] Support PostgreSQL `bpchar` type and array of char type #29192

kujon commented Jul 22, 2020 •

edited

Loading

maropu commented Jul 23, 2020 •

edited

Loading

maropu commented Jul 23, 2020 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading