-
Notifications
You must be signed in to change notification settings - Fork 495
[repr types] Fallback to repr type checks before failing #34958
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
[repr types] Fallback to repr type checks before failing #34958
Conversation
889b157 to
49786cd
Compare
|
For the test fails:
|
src/expr/src/scalar.rs
Outdated
| && e1 | ||
| .typ(column_types) | ||
| .union(&e2.typ(column_types)) | ||
| .sql_union(&e2.typ(column_types)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and the below two changed calls, why not try_union? I think the important thing for these calls (see code comment above) is to match typ's handling of If, which does a proper repr union, not a sql_union.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thank you! I found two more instances in there. I thought I got all these... I must have not saved properly or something.
I'll trigger another nightly run including the relevant tests (sqllogictest with and without multiple replicas, testdrives) and then merge.
3235085 to
3785218
Compare
Recent (reverted) changes to
ColumnKnowledgeandenable_cast_eliminationtickled some assertions we have in our code that assume we will have SQL typechecking at the MIR level. This PR relaxes those checks to try for repr typing as a fallback, usingtracing::error!to notify when this fallback occurs (so we can track it in Sentry).There were two places these checks were happening:
SqlColumnType::union, which was frequently.unwrap()ed.normalize_lets.Scanning through 1055 assertions in the
expr,repr,sql, andtransformcrates found no others, nor were the 42 uses ofbase_eqacross the codebase suspect.Motivation
https://github.com/MaterializeInc/database-issues/issues/10045
https://github.com/MaterializeInc/database-issues/issues/10046
https://github.com/MaterializeInc/database-issues/issues/10052
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.