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

sql: refactor type types #10571

Closed
benesch opened this issue Feb 9, 2022 · 1 comment · Fixed by #10624
Closed

sql: refactor type types #10571

benesch opened this issue Feb 9, 2022 · 1 comment · Fixed by #10624
Labels
C-refactoring Category: replacing or reorganizing code

Comments

@benesch
Copy link
Member

benesch commented Feb 9, 2022

We've got a lot of confusion around our scalar types at the moment, between repr::ScalarType, pgrepr::Type, coord::catalog::Type, and postgres_types::Type. Here's a series of various refactorings I have in mind:

  • The abbreviated term "typmod" is used exclusively to refer to a PostgreSQL-style packed 32-bit integer describing how to modify a base type.
  • The term "modifiers" is used in the SQL parser to refer to a list of arbitrary Values attached to the type. These are unpacked type modifiers.
  • The code in repr usually speaks very concretely about e.g. the "max scale" of a numeric or the "max length" of a varchar. It may occasionally refer to this class of thing as "type modifiers", but it will be careful to avoid the shorthand "typmod".
  • We introduce wrapper types called NumericMaxScale and VarCharMaxLength and such that validate the bounds of the provided integer. These wrapper types offer infallible conversions to usize and i32 and whatever other types are convenient, and offer a fallible conversion from other numeric types.
  • The extract_typ_mods family of functions moves out repr and into sql. This code is exclusively used in the SQL parser -> ScalarType conversion, and so belongs in the planner, not repr.
  • The catalog exposes the TypeInner struct to the sql crate, so that the problem of converting a SQL parser data type into a ScalarType can live entirely in the SQL planner, rather than in a try_get_lossy_scalar_type_by_id method that lives in the catalog.
@benesch benesch added the C-refactoring Category: replacing or reorganizing code label Feb 9, 2022
@benesch
Copy link
Member Author

benesch commented Feb 10, 2022

cc @nharring-adjacent @sploiselle

No need to take any action on this mega issue, but I'm going to periodically slice up follow-up tasks and ask for help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-refactoring Category: replacing or reorganizing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant