sql: cast derived to proposed types in WMR CTEs #23658
Conversation
There was a problem hiding this comment.
This is impressive and fast work, @sploiselle—thank you so much! 🙇🏽
Someone else from surfaces should take a deeper look, but I read through the design and implementation in medium detail and it all checked out for me.
| ## Solution Proposal | ||
|
|
||
| We can cast the derived `RelationExpr`'s output types to match those that the | ||
| user proposed in their statement using `CastContext::Assignment`. |
| casting `numeric(38,10)` to `numeric(38,2)` (such as adding two values of these | ||
| types) will produce `numeric(38,10)`––there is no way of performing the | ||
| truncation expressed by the typmod to the higher-scaled value. Because of this, | ||
| I don't think the implicit context is appropriate to use when casting the |
| return any `numeric` type irrespective of its scale, e.g. `numeric(38,0)`, | ||
| `numeric(38,2)`. | ||
|
|
||
| This might be the "easiest" to implement, but introduces a type of |
| WITH MUTUALLY RECURSIVE | ||
| foo (a int, b int) AS (SELECT 1 UNION SELECT a FROM bar), | ||
| bar (a int) as (SELECT a FROM foo) | ||
| SELECT a FROM foo, bar; | ||
|
|
||
| ## Test with more columns than declared | ||
| statement error did not match inferred type | ||
| statement error db error: ERROR: WITH MUTUALLY RECURSIVE query "foo" declared types \(integer, integer\), but query returns types \(integer, integer, integer\) |
There was a problem hiding this comment.
Does this show typmods properly? I know the whole inspiration behind this work is that numeric typmods are now automatically cast, but I'm curious whether providing an integer when, say, a numeric(1, 2) is required properly renders the numeric(1, 2).
There was a problem hiding this comment.
We don't include the typmod, which is a feature/limitation of humanize_scalar_type––it only refers to the type objects. With the assignment-type casts in place, this makes sense because the errors have to be more dramatic than the typmods being mismatched (i.e. you gave us text and we needed a number). So including the typmod won't give folks meaningful context to fix the problem.
src/sql/src/plan/query.rs
Outdated
| }); | ||
|
|
||
| if !all_keys { | ||
| return err(proposed_typ, derived_typ); |
There was a problem hiding this comment.
I think this was pre-existing, but if these don't match, the error message will talk only about the scalar types when in fact it is a key mismatch, no?
There was a problem hiding this comment.
This was actually impossible to trigger because WMR CTEs don't support keys. Converted the existence of any key into an error so that if we support it, we'll have to fix this error (assuming we write a test 😄)
src/sql/src/plan/query.rs
Outdated
| .zip(proposed_typ.column_types.iter()) | ||
| { | ||
| if derived_col.nullable && !proposed_col.nullable { | ||
| return err(proposed_typ, derived_typ); |
There was a problem hiding this comment.
Ditto... do we properly render errors given nullability mismatches?
There was a problem hiding this comment.
Ditto above but we don't support NOT NULL in the CTE column definitions
We checked that the derived and proposed RelationTypes in WMR CTEs had compatible definitions for NOT NULL columns and keys. However, the proposed column types cannot contain the NOT NULL constraint, nor can we express keys. Convert the potentially confusing errors we returned previously to internal errors that should surface in testing when these features get added.
The code on
mainprevented you from usingnumericvalues with different scales interchangeably, though SQL supports the operation.cc @frankmcsherry
Motivation
This PR fixes a recognized bug. Slack
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.WITH MUTUALLY RECURSIVEcommon table expressions. For example, you can now returnNUMERICvalues of arbitrary scales (e.g.NUMERIC(38,2)for columns defined asNUMERIC; previously, this would error becauseNUMERIChas no scale applied andNUMERIC(38,2)does.