[repr types] analysis and MRE typechecker#34792
Conversation
|
The goal somewhat soon is that we should be able to remove the old |
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
LGTM, just minor comments
src/transform/src/analysis.rs
Outdated
| .expect("ReprRelationType analysis discovered type-less expression") | ||
| }); | ||
|
|
||
| let sql_typ = expr.try_repr_col_with_input_repr_cols(input_cols).unwrap(); |
There was a problem hiding this comment.
The variable name is stale from a copy-paste, I guess.
| ) => Ok(ReprScalarType::Range { | ||
| element_type: Box::new(element_type.union(other_element_type)?), | ||
| }), | ||
| (_, _) => bail!("Can't union scalar types: {:?} and {:?}", self, scalar_type), |
There was a problem hiding this comment.
I'm a bit worried that somebody will add a type and forget to handle it here. Maybe we could add an Arbitrary/proptest-based test that generates (a, a) pairs, and calls this function, and expects to not error? Plus maybe a big warning comment in ReprScalarType to not forget to add new variants to Arbitrary.
There was a problem hiding this comment.
Good thought! Added a proptest and appropriate notes on the enum.
2 participants
Adds direct generation of repr types via analysis (and the underlying
try_repr_col_with_input_repr_cols) function. Will allow us to replace calls to.typ()with calls to.repr_typ(), used in many transforms.Motivation
design doc: MIR typechecking using representation types #27239
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.