sql: refactor type types #10624
sql: refactor type types #10624
Conversation
benesch
force-pushed
the
repr-improvements
branch
3 times, most recently
from
4bdee16
to
21943fc
Compare
| } | ||
|
|
||
| impl NumericConstraints { | ||
| fn from_typmod(typmod: i32) -> Option<NumericConstraints> { |
There was a problem hiding this comment.
🤯 This seems like wants to share an implementation w/
materialize/src/expr/src/scalar/func.rs
Line 5691 in e49579c
Once this PR lands, will see if I can find a place to tie these together.
There was a problem hiding this comment.
Oh 100% it should!! Good catch!
There was a problem hiding this comment.
I obviously didn't spend time reverse-engineering the motivation behind all of these changes. However, I "spot checked" all of the major points of refactoring and things seem really great/clear.
If you have the time or interest, I'd love to hear your high-level thinking behind:
PgScalarTypebeing the unnecessary pivot betweenpgreprandScalarType. I believe you mentioned removing this to me previously, but did you know it was chaff because you had to write it originally, or was this something you discovered by means of "looking at"/understanding the dependency graph?- New typing the typmods in
adt. Like, is it nice to tidy up the semantic boundaries of these things? I saw a "greatest common denominator" for all of them, so never would have been motivated to specialize the code. Would love to get a clearer sense of why you think this approach is superior (asking to learn, not in any way a disagreement).
| /// [`VARHDRSZ`](https://github.com/postgres/postgres/blob/REL_14_0/src/include/c.h#L627) constant | ||
| const PG_HEADER_SIZE: i32 = 4; | ||
|
|
||
| pub enum PgScalarType { |
There was a problem hiding this comment.
This all looks reasonable, I mostly focused on the postgres_util and pgrepr bits since I'm more familiar with that code however everything is clear and readable. Thanks very much for doing this, these type relationships seem demonstrably easier to grok and use.
A combination of both, I think! When I wrote
Oh, I just get stressed out about having representable states that are invalid. Like, nothing stopped someone from inadvertently constructing |
benesch
force-pushed
the
repr-improvements
branch
from
21943fc
to
a055ebc
Compare
They don't seem to be used. Also improve the comments on the wrapper types.
…types
Redistribute how PostgreSQL types are handled. Creating a PostgreSQL
type from an OID and typmod is now handled entirely by `pgrepr::Type`.
The `postgres_util::PgColumn` struct now stores a `pgrepr::Type`
directly, rather than a separate `PgScalarType` enum that was relatively
duplicative with `pgrepr::Type`.
The `sql` crate now generates views for a PostgreSQL source by
converting a `pgrepr::Type` to an `UnresolvedDataType` by way of a new
`fmt::Display` implementation on `pgrepr::Type`.
For safety, the modifiers on `ScalarType::{Numeric,VarChar,Char}` now
have newtype wrappers that enforce the bounds at construction. For
clarity, the `scale` parameter of `ScalarType::Numeric` has been renamed
`max_scale` and the `length` parameter of `ScalarType::VarChar` has been
renamed `max_length`, to better reflect that they are maximums, not hard
requirements. This is in contrast to `ScalarType::Char`, where the
length is in fact a hard requirement, not a maximum.
benesch
force-pushed
the
repr-improvements
branch
from
a055ebc
to
1776c9b
Compare
|
I had to ditch the last commit that updated the test that compares PostgreSQL OIDs to Materialize OIDs, because it was failing in CI for reasons entirely unrelated to this commit. I'll do that in a separate PR to get this unblocked. |
Store types in the catalog via a new CatalogType enum. This enum is nearly one-to-one with ScalarType, except that modifier fields are removed and embedded types are replaced with GlobalId references. This type is used throughout the SQL planner, in the plans returned by `CREATE TYPE` and in the type name resolution code. The motivation for this refactor is the removal of the "lossy" conversions from `pgrepr::Type` to `ScalarType`. Now, all conversions are either full fidelity or report an error if they would discard data.
benesch
force-pushed
the
repr-improvements
branch
from
b4c7572
to
6141324
Compare
|
Hallelujah! Thanks for the reviews, everyone! |
This mega PR refactors the type types as described in #10571. More details in that issue and in the commit messages within. The net result is, IMO, a set of types whose purposes are more clear, safer and more correct conversions between them, and the removal of a lot of duplicate code.
I'm sorry this diff is so large, but these changes were massively interdependent, and I don't have any better ideas for further splitting it into chunks.
Fix #10571.
Fix #10576.
Motivation
Checklist