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

Document PostgreSQL type mapping #11378

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Mar 9, 2022

Description

Document PostgreSQL type mapping

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

Screen Shot 2022-03-09 at 15 06 46

@cla-bot cla-bot bot added the cla-signed label Mar 9, 2022
@mosabua
Copy link
Member

mosabua commented Mar 9, 2022

I cant comment on technical accuracy but typically there are different mappings for read vs create table style operations.

https://trino.io/docs/current/connector/oracle.html#type-mapping

And we probably should also import https://trino.io/docs/current/connector/redshift.html#id1

@mosabua mosabua added the docs label Mar 9, 2022
@mosabua
Copy link
Member

mosabua commented Mar 9, 2022

The best wording that clarifies the two mappings can be found in https://docs.starburst.io/latest/connector/starburst-synapse.html#type-mapping .. we have plans to make all connectors in Trino consistent along those lines but have not gotten around to it. Maybe we can use this PR as model to establish a good baseline wording for this in Trino.

@ebyhr
Copy link
Member Author

ebyhr commented Mar 9, 2022

And we probably should also import https://trino.io/docs/current/connector/redshift.html#id1

The "General configuration properties" already exists in the PostgreSQL documentation.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Some of these comments are about adding more test coverage. I'll create separate issues for those.

``BIT`` ``BOOLEAN``
``SMALLINT`` ``SMALLINT``
``INTEGER`` ``INTEGER``
``BIGINT`` ``BIGINT``
Copy link
Member

Choose a reason for hiding this comment

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

This reminds that we should probably also test smallserial, serial and bigserial types too.

Copy link
Member

Choose a reason for hiding this comment

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

=================== ================================ =======================================================================
PostgreSQL Trino Notes
=================== ================================ =======================================================================
``BIT`` ``BOOLEAN``
Copy link
Member

Choose a reason for hiding this comment

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

PostgreSQL has a real boolean type too.

Also do we properly handle bit(n) in the connector?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also do we properly handle bit(n) in the connector?

I don't think so.

Copy link
Member

Choose a reason for hiding this comment

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

docs/src/main/sphinx/connector/postgresql.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/postgresql.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/postgresql.rst Outdated Show resolved Hide resolved
@ebyhr ebyhr force-pushed the ebi/docs-postgresql-type-mapping branch from 2f11f11 to b21e397 Compare March 9, 2022 06:04
@ebyhr ebyhr requested a review from hashhar March 9, 2022 06:07
@hashhar
Copy link
Member

hashhar commented Mar 9, 2022

ENUM is missing. We map that to a VARCHAR. There are tests for it.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % enum + time(n)

@ebyhr ebyhr force-pushed the ebi/docs-postgresql-type-mapping branch from b21e397 to f042995 Compare March 9, 2022 07:32
@ebyhr ebyhr merged commit c841980 into trinodb:master Mar 9, 2022
@ebyhr ebyhr deleted the ebi/docs-postgresql-type-mapping branch March 9, 2022 08:46
@github-actions github-actions bot added this to the 373 milestone Mar 9, 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.

Add type mapping documentation for PostgreSQL connector
3 participants