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 casts to be more like postgresql #55094

Open
otan opened this issue Oct 1, 2020 · 5 comments
Open

sql: refactor casts to be more like postgresql #55094

otan opened this issue Oct 1, 2020 · 5 comments
Labels
A-sql-typing SQLtype inference, typing rules, type compatibility. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@otan
Copy link
Contributor

otan commented Oct 1, 2020

Our current method of handling casts and operators are a bit of a mess.

Casts are defined in a massive switch statement, and their volatility in a separate map (validCasts).

In large parts, AdjustValueFor[Column]Type should be folded into here.

We should refactor casts and parse to be more like Postgres: there should be a map that roughly goes map[fromType oid.Oid]map[toType oid.Oid]castOps that defines casts from and to. They should be able to re-use the parse function -- strings should automatically be populated into these cast maps using the parse definitions. In postgres, this is handled by a <datatype>_in command.

Doing this will also greatly simplify adding new types.

Jira issue: CRDB-3696

@otan otan added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-typing SQLtype inference, typing rules, type compatibility. labels Oct 1, 2020
craig bot pushed a commit that referenced this issue Nov 24, 2020
55095: tree: fix casts and parse to obey type limits r=rafiss a=otan

Resolves #54844

A more general solution for this should be #55094.

Release note (bug fix): Previously, casts and parsing of strings to
types may allow an out-of-bounds value to be successfully used (e.g.
SELECT -232321321312::int2 would work), but fail with an out-of-bounds
message when it is inserted into the table. This has been rectified to
instead be checked when the value is parsed or being casted to.

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
@mgartner
Copy link
Collaborator

mgartner commented Sep 1, 2021

@otan I'm considering making this refactor to make some upcoming changes to casting easier. I had a couple questions:

there should be a map that roughly goes map[fromType oid.Oid]map[toType oid.Oid]castOps that defines casts from and to

What's the reasoning behind using an OID -> OID map instead of a Family -> Family map? It seems like OID -> OID could cause a lot of duplicate code and boilerplate.

They should be able to re-use the parse function -- strings should automatically be populated into these cast maps using the parse definitions.

Can you explain more what you mean by this?

@otan
Copy link
Contributor Author

otan commented Sep 1, 2021

It seems like OID -> OID could cause a lot of duplicate code and boilerplate.

Well, it doesn't if you have implicit/assignment casts :). OID -> OID is most postgres-like, and the Family:Family stuff is a huge mess when you look at the various different times of String-family OIds we have. However, unless we do the type system refactor I think Family:Family makes sense (for now).

They should be able to re-use the parse function -- strings should automatically be populated into these cast maps using the parse definitions.

I phrased this badly, let me rephrase. This is in the context of string casts, which IIUC goes through ParseAndRequireString anyway.

In PG, type I/O input/output functions depend on typin and typout in pg_type which I believe calls into the C code using some dlopen magic: https://github.com/postgres/postgres/blob/537ca68dbb2463f7b1c44e9466b8fbdd7505b2e1/src/backend/utils/adt/int.c#L265-L283

What I believe I meant was that the casts should re-use the ParseD.... functions to most strongly simulate this, but I think ::string and string:: related casts already have some variation of this magic (which of course is wrong for e.g. char types which fail to truncate or what not).

@mgartner
Copy link
Collaborator

mgartner commented Sep 7, 2021

Well, it doesn't if you have implicit/assignment casts :)

I don't follow. OID -> OID would require a map that contains casting details for each OID pair. Family -> Family would by definition be a smaller map.

However, unless we do the type system refactor I think Family:Family makes sense (for now).

Sounds good, I'll give it a shot.

@otan
Copy link
Contributor Author

otan commented Sep 7, 2021

Family -> Family would by definition be a smaller map.

smaller map, a lot more if conditions based on oids ;)

@otan otan added this to Triage in SQL Sessions - Deprecated via automation Dec 8, 2021
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Dec 8, 2021
@otan otan moved this from Triage to Longer term backlog in SQL Sessions - Deprecated Dec 8, 2021
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-typing SQLtype inference, typing rules, type compatibility. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
Development

No branches or pull requests

2 participants