Skip to content

[SPARK-10655][SQL] Adding additional data type mappings to jdbc DB2dialect. - #9162

Closed
sureshthalamati wants to merge 3 commits into
apache:masterfrom
sureshthalamati:db2dialect_enhancements-spark-10655
Closed

[SPARK-10655][SQL] Adding additional data type mappings to jdbc DB2dialect.#9162
sureshthalamati wants to merge 3 commits into
apache:masterfrom
sureshthalamati:db2dialect_enhancements-spark-10655

Conversation

@sureshthalamati

@sureshthalamatisureshthalamati commented Oct 19, 2015

Copy link
Copy Markdown
Contributor

This patch adds DB2 specific data type mappings for decfloat, real, xml , and timestamp with time zone (DB2Z specific type) types on read and for byte, short data types on write to the to jdbc data source DB2 dialect. Default mapping does not work for these types when reading/writing from DB2 database.

Added docker test, and a JDBC unit test case.

@rxin

rxin commented Oct 20, 2015

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

@SparkQA

Copy link
Copy Markdown

Test build #1930 has finished for PR 9162 at commit ec8e546.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't this mapping it to normal decimal with precision 31?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i.e. precision lost here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Using this mapping DB2 will throw error if the precision of the value being written is > 31 during insert execution. But if the value scale is higher than 2 , value will rounded to scale of 2.

Other alternative was to let it fail with error when create table is executed (current behavior). I was not sure how common the decimal type system default (38, 18) is used to create the data frame. I thought it is better to map to DB2 max precision, instead of failing with error ,considering there is no way to write decimals of precision > 31 to DB2.

If you think it is better to fail on create , instead of surprises during execution. I will update the patch.

I am also working on creating pull request (SPARK-10849) to allow users to specify the target database column type. This will allow user to specify the decimal precision, and scale of their choice in these kind of scenarios.

@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

Thank you for reviewing patch , Reynold. Please let me know if it needs any more changes.

@rick-ibm

Copy link
Copy Markdown
Contributor

I think these are fine defaults. The work on SPARK-10849 gives fine-grained control to power users. LGTM. Thanks.

@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

@rxin Wondering if I need any additional work on this patch ? Thanks.

@JoshRosen

Copy link
Copy Markdown
Contributor

Hey @sureshthalamati, take a look at #9503, which we just merged to re-enable the Docker JDBC integration tests; you should be able to build on that patch to write integration tests that run against a real DB2 instance that's running in Docker.

@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

Thank you for the information , Josh. Luciano seems to working on creating PR for DB2 docker test setup. I will check with him , and incorporate the type mapping tests into the docker test case.

@lresende

@sureshthalamati
sureshthalamatiforce-pushed the db2dialect_enhancements-spark-10655 branch from ec8e546 to 729fb85CompareNovember 25, 2015 23:03
@lresende

Copy link
Copy Markdown
Member

I have a wip pr #9893, and an open question that I think @JoshRosen could help about the JDBC drivers for the other docker tests. Anyway, these two PRs are independent of each other, no ?

@sureshthalamati
sureshthalamatiforce-pushed the db2dialect_enhancements-spark-10655 branch from 27eeba0 to 5d022beCompareMay 21, 2016 00:26
@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

@JoshRosen@rxin

Updated the PR with the DB2 docker test case for the DB2 specific type mappings added in this PR. TIME STAMP WITH TIME ZONE is DB2Z specific , I could not add the docker test case for this data type.

Can you please review the updated PR.

@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

ping @JoshRosen@rxin

This issue was blocked due to docker relates issues for a while. If you can review the updated PR , that will be great.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a duplication of BooleanType line here ? probably copy + paste issue.

@lresende

Copy link
Copy Markdown
Member

Other then the minor comment around BooleanType, it LGTM.

@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

Thanks, Luciano. Addressed your comment.

@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

@JoshRosen@rxin Any suggestions to improve this fix to get it merged ?

@lresende

Copy link
Copy Markdown
Member

Jenkins retest this please

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about DECFLOAT(16) and DECFLOAT(34)?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for reviewing @gatorsmile . Added test cases for those two variations of the DECFLOAT types.

@SparkQA

Copy link
Copy Markdown

Test build #65932 has finished for PR 9162 at commit f85c3d9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • assert(types(8).equals(\"class java.math.BigDecimal\"))
    • assert(types(9).equals(\"class java.math.BigDecimal\"))

@gatorsmile

Copy link
Copy Markdown
Member

@sureshthalamati Could you please retest it using DB2?

@gatorsmile

Copy link
Copy Markdown
Member

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #78198 has finished for PR 9162 at commit f85c3d9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • assert(types(8).equals(\"class java.math.BigDecimal\"))
  • assert(types(9).equals(\"class java.math.BigDecimal\"))

@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

sure @gatorsmile . Thanks.

@sureshthalamati
sureshthalamatiforce-pushed the db2dialect_enhancements-spark-10655 branch from f85c3d9 to 8b8bc9aCompareJune 21, 2017 00:31
@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

@gatorsmile I rebased and ran the DB2 docker test on my machine, it ran fine.

@SparkQA

Copy link
Copy Markdown

Test build #78337 has finished for PR 9162 at commit 8b8bc9a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • assert(types(8).equals(\"class java.math.BigDecimal\"))
  • assert(types(9).equals(\"class java.math.BigDecimal\"))

@gatorsmile

Copy link
Copy Markdown
Member

The scope of this PR is just to the DB2 dialect. The risk is pretty small even if the fix might not cover all the scenarios. Thanks! @sureshthalamati

LGTM

@gatorsmile

Copy link
Copy Markdown
Member

Thanks! Merging to master.

@sureshthalamati

Copy link
Copy Markdown
ContributorAuthor

Thank you @gatorsmile

robert3005 pushed a commit to palantir/spark that referenced this pull request Jun 29, 2017
…alect.
This patch adds DB2 specific data type mappings for decfloat, real, xml , and timestamp with time zone (DB2Z specific type) types on read and for byte, short data types on write to the to jdbc data source DB2 dialect. Default mapping does not work for these types when reading/writing from DB2 database.
Added docker test, and a JDBC unit test case.
Author: sureshthalamati <suresh.thalamati@gmail.com>
Closesapache#9162 from sureshthalamati/db2dialect_enhancements-spark-10655.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@sureshthalamati@rxin@SparkQA@rick-ibm@JoshRosen@lresende@gatorsmile