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

Extended readMapping to columnMapping for jdbc connectors #17972

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pratyakshsharma
Copy link
Contributor

@pratyakshsharma pratyakshsharma commented Jul 4, 2022

Currently jdbc connectors control how the data should be read using ReadMapping. But the writing is hard coded in JdbcPageSink#appendColumn. Below are some of the requirements which require proper writeMapping and inclusion of write functions with read mapping (or a generic ColumnMapping) -

  • Add support for array data type in Postgres - This needs to be mapped as a presto array type as well as presto json type due to certain trade offs
  • Add support for how unsupported jdbc type columns should be handled.
  • Respect original data type while inserting (needed to support inserting postgres array values when they are mapped to presto json.)
  • Also changes are needed to allow inserting into tables when one of the columns has been ignored as part of unsupported type handling.

All this requires us to introduce a new WriteMapping for writing data to storage. ReadMapping needs to be extended into ColumnMapping which will also contain WriteFunctions. A new WriteFunction interface needs to be introduced.

Ref: #12151

Test plan - (Please fill in how you tested your changes)

Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines. Don't forget to follow our attribution guidelines for any code copied from other projects.

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== NO RELEASE NOTE ==

@pratyakshsharma pratyakshsharma requested a review from a team as a code owner July 4, 2022 09:22
@pratyakshsharma
Copy link
Contributor Author

@NikhilCollooru gentle ping!

@pratyakshsharma pratyakshsharma changed the title [WIP]: Extended readMapping to columnMapping for jdbc connectors Extended readMapping to columnMapping for jdbc connectors Sep 14, 2022
@pratyakshsharma
Copy link
Contributor Author

@imjalpreet @agrawalreetika @fgwang7w

Can you guys take the first pass here?

@ethanyzhang
Copy link
Contributor

Can you revisit this? @pratyakshsharma

@pratyakshsharma
Copy link
Contributor Author

@agrawalreetika Let us revive this?

@pratyakshsharma
Copy link
Contributor Author

@agrawalreetika I have resolved the conflicts.

@agrawalreetika
Copy link
Member

Thank you @pratyakshsharma for raising the PR. I see there are multiple changes done in this PR. Could we break it into feature-wise commits? It would also make it easier to review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants