Skip to content
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.

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

[SPARK-12966][SQL] Support ArrayType(DecimalType) in Postgre JDBC #10898

Closed
wants to merge 3 commits into from

Conversation

maropu
Copy link
Member

@maropu maropu commented Jan 25, 2016

The current master throws an exception below;

org.postgresql.util.PSQLException: Unable to find server array type for provided name decimal(38,18)

This pr fixes this issue.

@SparkQA
Copy link

SparkQA commented Jan 25, 2016

Test build #49989 has finished for PR 10898 at commit 304d3d0.

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

@maropu
Copy link
Member Author

maropu commented Jan 25, 2016

retest this please

@blbradley
Copy link
Contributor

You should not be converting to doubles when testing BigDecimal or DecimalType..

@blbradley
Copy link
Contributor

Also, we should be handling the precision and scale returned from Postgres. I've looked deep enough to see that this is possible.

@maropu
Copy link
Member Author

maropu commented Jan 25, 2016

ISTM precision and scale returned by postgres are filled with ResultSetMetaData.getPrecision and ResultSetMetaData.getScale in JDBCRDD.

val c13Expected = Seq(1.5, 3.25).map(new java.math.BigDecimal(_))
assert(rows(0).getSeq[java.math.BigDecimal](13).zipWithIndex.forall { case (v, idx) =>
v.compareTo(c13Expected(idx)) == 0
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not follow the style of the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like

assert(rows(0).getSeq(13) == Seq[BigDecimal](0.11, 0.22))

Copy link
Member Author

Choose a reason for hiding this comment

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

The test fails because the behaviour of BigDecimal#equals and BigDecimal#compareTo are different.

@SparkQA
Copy link

SparkQA commented Jan 25, 2016

Test build #49993 has finished for PR 10898 at commit 304d3d0.

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

@blbradley
Copy link
Contributor

@maropu Indeed, but they are not available in the metadata pased to dialect.getCatalystType. They probably need to be added to the metadata and logic added to PostgresDialect to handle that.

@maropu
Copy link
Member Author

maropu commented Jan 25, 2016

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jan 25, 2016

Test build #49995 has finished for PR 10898 at commit 3626132.

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

@SparkQA
Copy link

SparkQA commented Jan 25, 2016

Test build #49998 has finished for PR 10898 at commit 3626132.

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

@maropu
Copy link
Member Author

maropu commented Jan 28, 2016

I found that it's not easy to support this type in postgresql in the current interface of JdbcDialect.
This is because the postgresql-jdbc implementation cannot handle a decimal type name with precision and scale such as DECIMAL(xx, xx) in Connection#createArrayOf.
To fix this issue, we can add a specific entry in PostgresDialect#getJDBCType (https://github.com/apache/spark/pull/10898/files#diff-23da60722bc6bfc160cbb59bd99f5925R64).
However, this fix makes things worse; JdbcUtils#schemaString cannot pass precision and scale for Decimal when relation defined in postgresql through DataFrameWriter.

I'm not sure how to fix this though, it seems okay to just add TODO comments and throw an unsupported exception for this type for the time being.
cc: @marmbrus

@SparkQA
Copy link

SparkQA commented Jan 28, 2016

Test build #50251 has finished for PR 10898 at commit 52eaebe.

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

@blbradley
Copy link
Contributor

@maropu I have submitted an implementation of this in #10928 and am not getting the error you describe.

@blbradley
Copy link
Contributor

@maropu I see a corner case in schemaString. I believe I can get the right behavior now.

@maropu
Copy link
Member Author

maropu commented Jan 29, 2016

I'll close this pr and I discuss this in #10928.

@maropu maropu deleted the SPARK-12747 branch July 5, 2017 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants