Skip to content

Treat other SQL types as varchar#12141

Closed
stewartpark wants to merge 1 commit intoprestodb:masterfrom
stewartpark:cast-as-varchar
Closed

Treat other SQL types as varchar#12141
stewartpark wants to merge 1 commit intoprestodb:masterfrom
stewartpark:cast-as-varchar

Conversation

@stewartpark
Copy link
Contributor

@stewartpark stewartpark commented Dec 28, 2018

Continuation of #7506
Fixes #5649 . Fixes #11821
Also mentioned in #5989 #6614

As discussed in the previously closed PR, I think this can serve as a decent quick first step. want to continue the work needed for this. But unfortunately as is, as @electrum predicted in the other PR, INSERT doesn't work. Not sure how we want to proceed from here. Is creating a new type(UUID, etc) that extends AbstractType the way to go?

@electrum
Copy link
Contributor

electrum commented Dec 29, 2018

While this is easy to do, the reason we haven't done it is that switching to first-class types in the future would break existing usages. For example, if we add a UUID type in the future and map it to the PostgreSQL UUID type, it would likely break existing queries. Additionally, some types do not have a usable string representation, or are better mapped as numbers, etc.

On the other hand, not being able to use certain types from Presto is definitely a usability problem. I think it would help to break this down:

  • What specific types (in which databases) are we interested in supporting?
  • Do we care about supporting user-defined types? If so, how should the mapping be handled?

@stewartpark
Copy link
Contributor Author

stewartpark commented Dec 29, 2018

While this is easy to do, the reason we haven't done it is that switching to first-class types in the future would break existing usages. For example, if we add a UUID type in the future and map it to the PostgreSQL UUID type, it would likely break existing queries.

@electrum that makes sense. I need to use UUID, hstore, JSONB fields in PostgreSQL. I don't think supporting these natively as a separate type is a requirement. Providing a way for users to be able to deal with the non-standard types as a compatible value is a reasonable first step to help with the usability problem. I wonder if we can make a connector-specific type/logic that converts into a standard type so each connector can deal with non-standard stuff, and the knowledge of how to convert the values back and forth(or could be read-only) is contained in the container. What do you think? Or do you have any plan for this in mind?

@findepi
Copy link
Contributor

findepi commented Dec 29, 2018

@stewartpark would you consider a workaround? You can define a view (on the PostgreSQL side) over your table that casts unsupported data types (uuid, jsonb, etc) to varchar.
This way there is no problem with forward compatibility and insert semantics are controlled on the PostgreSQL side.

@stewartpark
Copy link
Contributor Author

stewartpark commented Dec 29, 2018

@findepi Thanks for the suggestion! I'm aware of the workaround that was discussed in the Google Groups but I don't think that's too ideal because that would make the application databases have to have an extra thing to maintain(schema changes, etc) for an external job. Do you have any concerns of having an interface/override for each jdbc-based connector to define what standard type a non-standard type(OTHER) should be interpreted into? (e.g. maybe overriding toPrestoType/toSqlType in the postgres connector and interpreting OTHER there can be one easy solution?)

@findepi
Copy link
Contributor

findepi commented Dec 29, 2018

What are your concerns of having an interface for each jdbc-based connector to define what standard type a non-standard type(OTHER) should be interpreted into?

I am not sure what you mean.
Each jdbc-based connector already defines the type mapping -- either by inheriting BaseJdbcClient#toPrestoType or overriding it.

@stewartpark
Copy link
Contributor Author

@findepi Oh yeah! I was looking at the code and was adding more to my comment. Do you think overriding that to provide some level of usability on specific custom types something Presto wants as an addition and I can contribute to?

@findepi
Copy link
Contributor

findepi commented Dec 29, 2018

@stewartpark sure! We want to support "custom types" on per-connector basis.
Note that this may not be as trivial at it sounds, especially where inserts and predicate push down are in play.
There are already issues covering certain types. eg Json -- #11913; arrays -- #11422.

@stale
Copy link

stale bot commented Jun 27, 2019

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

@stale stale bot added the stale label Jun 27, 2019
@stewartpark stewartpark deleted the cast-as-varchar branch June 28, 2019 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Special jdbc types (like postgres jsonb, interval, tsrange, ...) are skipped in jdbcTypeToPrestoType Add support for Postgres JSON type

4 participants