presto icon indicating copy to clipboard operation
presto copied to clipboard

Extended readMapping to columnMapping for jdbc connectors

Open pratyakshsharma opened this issue 2 years ago • 2 comments

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: https://github.com/prestodb/presto/pull/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 avatar Jul 04 '22 09:07 pratyakshsharma

@NikhilCollooru gentle ping!

pratyakshsharma avatar Jul 14 '22 08:07 pratyakshsharma

@imjalpreet @agrawalreetika @fgwang7w

Can you guys take the first pass here?

pratyakshsharma avatar Sep 14 '22 12:09 pratyakshsharma

Can you revisit this? @pratyakshsharma

ethanyzhang avatar Oct 13 '22 17:10 ethanyzhang

@agrawalreetika Let us revive this?

pratyakshsharma avatar Jan 12 '23 05:01 pratyakshsharma

@agrawalreetika I have resolved the conflicts.

pratyakshsharma avatar Mar 06 '23 09:03 pratyakshsharma

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

agrawalreetika avatar Mar 10 '23 07:03 agrawalreetika