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

Add database type alias information to database type endpoint #1037

Closed
dmos62 opened this issue Feb 2, 2022 · 12 comments
Closed

Add database type alias information to database type endpoint #1037

dmos62 opened this issue Feb 2, 2022 · 12 comments
Assignees
Labels
affects: architecture Improvements or additions to architecture type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@dmos62
Copy link
Contributor

dmos62 commented Feb 2, 2022

Problem

We have some db types that are equivalent. In such cases, we want to tell the frontend which of those types is the default or canonical one, so as to prevent it from having to hardcode it. E.g. DECIMAL and NUMERIC are different in the SQL standard, but equivalent in Postgres and thus equivalent in Mathesar.

Proposed solution

Off the top of my head I see two approaches:

  1. Define a type alias as being the non-canonical equivalent types and append the following attribute in returned JSON to aliases 'alias-of': 'SOME CANONICAL TYPE'.

  2. Define all equivalent types, canonical and not, as aliases and append an aliases attribute containing a list of the rest of the equivalent types to all aliases ('aliases': ['SOME OTHER ALIAS', 'SOME YET ANOTHER ALIAS']), then on each alias append whether it's canonical or not: 'is_canonical_alias': true | false.

This is in part about whether to consider all equivalent types as aliases of one another (e.g. NUMERIC and DECIMAL are aliases of one another), or whether to say that the alias is the non-canonical type, while the canonical type is not the alias of its equivalent types (e.g. NUMERIC is an alias of DECIMAL, but not the other way around). The latter option is less verbose, in that you just add the alias-of attribute to non-canonicals.

Additional context

Proposed here: #1035 (comment)

@dmos62 dmos62 added type: enhancement New feature or request affects: architecture Improvements or additions to architecture work: backend Related to Python, Django, and simple SQL status: draft labels Feb 2, 2022
@dmos62 dmos62 added this to the [06] Working with Tables milestone Feb 2, 2022
@dmos62 dmos62 self-assigned this Feb 2, 2022
@kgodey
Copy link
Contributor

kgodey commented Feb 2, 2022

I like option 1 because the frontend will have a "canonical" type to default to when a user is creating a new column.

@mathemancer
Copy link
Contributor

A couple of quick notes/details:

  • There's a "canonical SQL name" for each type available from the pg_catalog.format_type system function. (we'd have to do a bit of mapping from aliases to their OIDs to look this name up).
  • Oddly, while it's not too hard to find the OID of, e.g., VARCHAR, I can't figure out how to find the OID of DECIMAL. Hmm.

@dmos62
Copy link
Contributor Author

dmos62 commented Feb 4, 2022

@mathemancer Do you think this is worth reflecting? I think at the moment the only types that are equivalent are DECIMAL and NUMERIC, so let's hardcode it backend.

By the way, I'd say that DECIMAL should be canonical. The difference between an integer and a "numeric" is ambiguous, but the difference between an integer and a decimal is obvious.

@kgodey
Copy link
Contributor

kgodey commented Feb 4, 2022

@dmos62 Those are not the only types that are equivalent, there's also VARCHAR and TEXT, IIRC. Please do some research and find any others.

We should use whichever type Postgres uses as canonical as canonical in our API.

@mathemancer
Copy link
Contributor

mathemancer commented Feb 7, 2022

Some examples are:

  • CHARACTER and CHAR
  • CHARACTER VARYING and VARCHAR
  • BOOL and BOOLEAN

Maybe more. I really think we should reflect; I'm not sure how else we can be confident we've caught most cases.

Edit: @kgodey I don't think we should use TEXT and VARCHAR as aliases; they have different OIDs.

@kgodey
Copy link
Contributor

kgodey commented Feb 7, 2022

@mathemancer I guess I didn't recall correctly. We still need a way to let the frontend know that the options for TEXT and VARCHAR are the same, though (unless they're different? If so, how are they different?)

@dmos62
Copy link
Contributor Author

dmos62 commented Feb 8, 2022

I agree that we'd want to reflect this in the long run. But, I'm still suggesting to hardcode it (for the time being), if that will save time in the short run.

@kgodey
Copy link
Contributor

kgodey commented Feb 8, 2022

Agreed on hardcoding if it will save time.

@mathemancer
Copy link
Contributor

VARCHAR takes a length type_option, whereas TEXT does not.

@kgodey
Copy link
Contributor

kgodey commented Feb 15, 2022

Ah okay, then they should not be aliases.

@mathemancer
Copy link
Contributor

After looking through the linked PR for this issue, I realized I (we) got distracted by whether we could show aliases, and how to do it, and we forgot to consider whether we should. I think if we're showing two equivalent types to the UI as options for setting the type of a column, that's a bug. We should only show one type string as an option for a given type, and that type string should be the key in the ischema_names dictionary.

@kgodey
Copy link
Contributor

kgodey commented Jun 2, 2022

I assume this was resolved by the type refactor, closing.

@kgodey kgodey closed this as completed Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: architecture Improvements or additions to architecture type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants