Skip to content

Respect original column type when inserting#1174

Merged
findepi merged 3 commits intotrinodb:masterfrom
kasiafi:InsertRespectOriginalColumnType
Sep 27, 2019
Merged

Respect original column type when inserting#1174
findepi merged 3 commits intotrinodb:masterfrom
kasiafi:InsertRespectOriginalColumnType

Conversation

@kasiafi
Copy link
Copy Markdown
Member

@kasiafi kasiafi commented Jul 24, 2019

In JDBC connectors, currently the insert target type for a column depends
on the Presto type chosen to represent the column's values.
This strategy doesn't support multiple original types mapped to the same
Presto type -- a single WriteMapping imposes a target type which not necessarily
fits them all, and so inserts may cause errors at finish time when trying to copy
the temporary table into the target table (unless the target database is able
to perform a coercion).
This is fixed by making INSERT aware of the target column types i.e. by using
the original column type in the temporary table and obtaining the write function
from the ColumnMapping.

This will allow to support inserting Postgres array values when they are mapped to Presto JSON (issue #682 addressed by #1148)

Fixes #338

@cla-bot cla-bot bot added the cla-signed label Jul 24, 2019
@kasiafi kasiafi force-pushed the InsertRespectOriginalColumnType branch 2 times, most recently from aeae9ac to 6c449bf Compare July 25, 2019 09:15
@findepi findepi requested a review from electrum July 28, 2019 11:40
@kasiafi kasiafi force-pushed the InsertRespectOriginalColumnType branch from 6c449bf to c2a5313 Compare July 28, 2019 15:09
@kasiafi kasiafi force-pushed the InsertRespectOriginalColumnType branch from c2a5313 to 5a4c812 Compare August 23, 2019 12:40
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.

The JdbcPageSink is JDBC-specifc, and so JdbcOutputTableHandle is.
It's surprising at first that the argument isn't cast by the caller but it's being cast here.

I would consider something like this: pass JdbcOutputTableHandle (as it was).
Inside constructor, check:

if (!handle.getJdbcColumnTypes().isPresent()) {
    ... the old way
}
else {
    ....  the new way
}

(This would be further simplified if we combine WriteFunction and WriteNullFunction into a single interface, but let's not do this in this PR.)

@kasiafi kasiafi force-pushed the InsertRespectOriginalColumnType branch from 5a4c812 to 52de269 Compare September 23, 2019 10:15
@kasiafi
Copy link
Copy Markdown
Member Author

kasiafi commented Sep 23, 2019

Applied comments. Travis fails repeatedly due to out of memory error.

In JDBC connectors, currently the insert target type for a column depends
on the Presto type chosen to represent the column's values.
This strategy doesn't support multiple original types mapped to the same
Presto type -- a single `WriteMapping` imposes a target type which not necessarily
fits them all, and so inserts may cause errors at finish time when trying to copy
the temporary table into the target table (unless the target database is able
to perform a coercion).
This is fixed by making INSERT aware of the target column types i.e. by using
the original column type in the temporary table and obtaining the write function
from the `ColumnMapping`.
@kasiafi kasiafi force-pushed the InsertRespectOriginalColumnType branch from 52de269 to 290b6c0 Compare September 26, 2019 05:43
@findepi findepi merged commit 10cf82d into trinodb:master Sep 27, 2019
@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 27, 2019

Merged, thanks!

@pratyakshsharma
Copy link
Copy Markdown

@findepi @kasiafi I was trying to understand the changes in this PR. Referring to this statement - This strategy doesn't support multiple original types mapped to the same Presto type -- a single WriteMapping imposes a target type which not necessarily fits them all, and so inserts may cause errors at finish time when trying to copy the temporary table into the target table (unless the target database is able to perform a coercion)., can you give an example of the scenario or data type where the inserts would fail? Thanks!

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 17, 2022

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

Development

Successfully merging this pull request may close these issues.

When writing to existing table with JDBC-based connector, temporary table should match destination table's column types

3 participants