[SPARK-34333][SQL] Fix PostgresDialect to handle money types properly by sarutak · Pull Request #31442 · apache/spark

sarutak · 2021-02-02T17:35:55Z

What changes were proposed in this pull request?

This PR changes the type mapping for money and money[] types for PostgreSQL.
Currently, those types are tried to convert to DoubleType and ArrayType of double respectively.
But the JDBC driver seems not to be able to handle those types properly.

pgjdbc/pgjdbc#100
pgjdbc/pgjdbc#1405

Due to these issue, we can get the error like as follows.

money type.

[info]   org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 0.0 failed 1 times, most recent failure: Lost task 0.0 in stage 0.0 (TID 0) (192.168.1.204 executor driver): org.postgresql.util.PSQLException: Bad value for type double : 1,000.00
[info] 	at org.postgresql.jdbc.PgResultSet.toDouble(PgResultSet.java:3104)
[info] 	at org.postgresql.jdbc.PgResultSet.getDouble(PgResultSet.java:2432)
[info] 	at org.apache.spark.sql.execution.datasources.jdbc.JdbcUtils$.$anonfun$makeGetter$5(JdbcUtils.scala:418)

money[] type.

[info]   org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 0.0 failed 1 times, most recent failure: Lost task 0.0 in stage 0.0 (TID 0) (192.168.1.204 executor driver): org.postgresql.util.PSQLException: Bad value for type double : $2,000.00
[info] 	at org.postgresql.jdbc.PgResultSet.toDouble(PgResultSet.java:3104)
[info] 	at org.postgresql.jdbc.ArrayDecoding$5.parseValue(ArrayDecoding.java:235)
[info] 	at org.postgresql.jdbc.ArrayDecoding$AbstractObjectStringArrayDecoder.populateFromString(ArrayDecoding.java:122)
[info] 	at org.postgresql.jdbc.ArrayDecoding.readStringArray(ArrayDecoding.java:764)
[info] 	at org.postgresql.jdbc.PgArray.buildArray(PgArray.java:310)
[info] 	at org.postgresql.jdbc.PgArray.getArrayImpl(PgArray.java:171)
[info] 	at org.postgresql.jdbc.PgArray.getArray(PgArray.java:111)

For money type, a known workaround is to treat it as string so this PR do it.
For money[], however, there is no reasonable workaround so this PR remove the support.

Why are the changes needed?

This is a bug.

Does this PR introduce any user-facing change?

Yes. As of this PR merged, money type is mapped to StringType rather than DoubleType and the support for money[] is stopped.
For money type, if the value is less than one thousand, $100.00 for instance, it works without this change so I also updated the migration guide because it's a behavior change for such small values.
On the other hand, money[] seems not to work with any value but mentioned in the migration guide just in case.

How was this patch tested?

New test.

SparkQA · 2021-02-02T18:47:22Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39381/

SparkQA · 2021-02-02T19:19:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39381/

SparkQA · 2021-02-02T20:42:06Z

Test build #134793 has finished for PR 31442 at commit 59dbb59.

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

srowen · 2021-02-03T14:39:36Z

docs/sql-migration-guide.md


 ## Upgrading from Spark SQL 3.1 to 3.2

+  - In Spark 3.2, money type in PostgreSQL table is converted to `StringType` and money[] type is not supported due to the JDBC driver for PostgreSQL can't handle those types properly.


Yeah, DecimalType fixes the accuracy problem with money, but can't account for units. I suppose we could one day support it as a struct of DecimalType and StringType for currency, but string seems fine now.
Why not a string array for a money array?

Why not a string array for a money array?

For money type, Spark SQL calls PgResultSet.getDouble causing the error.

[info] org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 0.0 failed 1 times, most recent failure: Lost task 0.0 in stage 0.0 (TID 0) (192.168.1.204 executor driver): org.postgresql.util.PSQLException: Bad value for type double : 1,000.00 [info] at org.postgresql.jdbc.PgResultSet.toDouble(PgResultSet.java:3104) [info] at org.postgresql.jdbc.PgResultSet.getDouble(PgResultSet.java:2432) [info] at org.apache.spark.sql.execution.datasources.jdbc.JdbcUtils$.$anonfun$makeGetter$5(JdbcUtils.scala:418)

So, we can avoid this issue by mapping money type to StringType to let Spark SQL call getString rather than getDouble.

For money[] type, on the other hand, the PostgreSQL's JDBC driver calls PgResultSet.toDouble internally.

[info] at org.postgresql.jdbc.PgResultSet.toDouble(PgResultSet.java:3104) [info] at org.postgresql.jdbc.ArrayDecoding$5.parseValue(ArrayDecoding.java:235) [info] at org.postgresql.jdbc.ArrayDecoding$AbstractObjectStringArrayDecoder.populateFromString(ArrayDecoding.java:122) [info] at org.postgresql.jdbc.ArrayDecoding.readStringArray(ArrayDecoding.java:764)

We can control how Spark SQL gets the value from the array obtained by PgResultSet.getArray, but it's difficult to control how the JDBC driver handles the elements in the array which is to be returned.

In the migration guide, we should also mention the previous behavior.

Thanks. I've updated.

sarutak · 2021-02-08T04:33:34Z

cc: @cloud-fan

cloud-fan · 2021-02-08T11:59:27Z

LGTM except one comment about migration guide.

…-money-type

SparkQA · 2021-02-09T04:05:33Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39642/

SparkQA · 2021-02-09T04:33:19Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39642/

SparkQA · 2021-02-09T06:56:24Z

Test build #135060 has finished for PR 31442 at commit c63121a.

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

maropu · 2021-02-10T03:45:18Z

Could you resolve the conflict? Looks fine otherwise.

maropu · 2021-02-10T04:06:50Z

docs/sql-migration-guide.md


 ## Upgrading from Spark SQL 3.1 to 3.2

+  - In Spark 3.2, PostgreSQL JDBC dialect uses StringType for MONEY and MONEY[] is not supported due to the JDBC driver for PostgreSQL can't handle those types properly. Previously, DoubleType and ArrayType of DoubleType are used respectively.


nit: Previously, => In Spark 3.1 or earlier,

sarutak · 2021-02-10T04:17:20Z

@maropu Thanks. I've updated.

SparkQA · 2021-02-10T05:28:37Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39673/

SparkQA · 2021-02-10T05:47:00Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39673/

SparkQA · 2021-02-10T06:59:11Z

Test build #135091 has finished for PR 31442 at commit b838218.

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

maropu · 2021-02-10T13:45:02Z

docs/sql-migration-guide.md


 ## Upgrading from Spark SQL 3.1 to 3.2

+  - In Spark 3.2, PostgreSQL JDBC dialect uses StringType for MONEY and MONEY[] is not supported due to the JDBC driver for PostgreSQL can't handle those types properly. In Spark 3.1 or earlier, DoubleType and ArrayType of DoubleType are used respectively.


I have the same comment here with #31491 (comment)

I've replied at #31491 (comment) .

@maropu Any more concern?

sarutak · 2021-02-16T02:23:17Z

If there is no objection, I'll merge soon.
If we find that the word dialect in the migration guide is an internal term, I'll open another PR to fix all the occurrence in the guide.

sarutak · 2021-02-17T01:52:31Z

Merged to master. Thanks all.

dongjoon-hyun · 2021-02-17T05:00:18Z

Thank you, @sarutak and all.

sarutak added 2 commits February 3, 2021 02:11

Fix for money and money[] types.

66d4292

Assign JIRA ID.

59dbb59

github-actions bot added DOCS SQL labels Feb 2, 2021

sarutak mentioned this pull request Feb 3, 2021

[SPARK-34343][SQL][TESTS] Add missing test for some non-array types in PostgreSQL #31456

Closed

srowen reviewed Feb 3, 2021

View reviewed changes

sarutak added 2 commits February 9, 2021 11:32

Merge branch 'master' of https://github.com/apache/spark into fix-for…

2cef36b

…-money-type

Add note about the previous behavior.

c63121a

maropu approved these changes Feb 10, 2021

View reviewed changes

Fix conflict.

7e8dfb8

maropu reviewed Feb 10, 2021

View reviewed changes

Change the expression in the migration guide.

b838218

maropu reviewed Feb 10, 2021

View reviewed changes

srowen approved these changes Feb 11, 2021

View reviewed changes

asfgit closed this in dd6383f Feb 17, 2021


		## Upgrading from Spark SQL 3.1 to 3.2

		- In Spark 3.2, money type in PostgreSQL table is converted to `StringType` and money[] type is not supported due to the JDBC driver for PostgreSQL can't handle those types properly.


		## Upgrading from Spark SQL 3.1 to 3.2

		- In Spark 3.2, PostgreSQL JDBC dialect uses StringType for MONEY and MONEY[] is not supported due to the JDBC driver for PostgreSQL can't handle those types properly. Previously, DoubleType and ArrayType of DoubleType are used respectively.

Conversation

sarutak commented Feb 2, 2021 • edited by maropu 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!

SparkQA commented Feb 2, 2021

Uh oh!

SparkQA commented Feb 2, 2021

Uh oh!

SparkQA commented Feb 2, 2021

Uh oh!

srowen Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak Feb 3, 2021 • 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!

cloud-fan Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak commented Feb 8, 2021

Uh oh!

cloud-fan commented Feb 8, 2021

Uh oh!

SparkQA commented Feb 9, 2021

Uh oh!

SparkQA commented Feb 9, 2021

Uh oh!

SparkQA commented Feb 9, 2021

Uh oh!

maropu commented Feb 10, 2021

Uh oh!

maropu Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak commented Feb 10, 2021

Uh oh!

SparkQA commented Feb 10, 2021

Uh oh!

SparkQA commented Feb 10, 2021

Uh oh!

SparkQA commented Feb 10, 2021

Uh oh!

maropu Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak commented Feb 16, 2021

Uh oh!

sarutak commented Feb 17, 2021

Uh oh!

dongjoon-hyun commented Feb 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sarutak commented Feb 2, 2021 •

edited by maropu

Loading

sarutak Feb 3, 2021 •

edited

Loading