Skip to content

Conversation

@mgree
Copy link
Contributor

@mgree mgree commented Feb 9, 2026

Recent (reverted) changes to ColumnKnowledge and enable_cast_elimination tickled 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, using tracing::error! to notify when this fallback occurs (so we can track it in Sentry).

There were two places these checks were happening:

  • In SqlColumnType::union, which was frequently .unwrap()ed.
  • In an assertion for normalize_lets.

Scanning through 1055 assertions in the expr, repr, sql, and transform crates found no others, nor were the 42 uses of base_eq across the codebase suspect.

Motivation

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@mgree mgree force-pushed the repr-types-check-with-fallback branch from 889b157 to 49786cd Compare February 9, 2026 19:47
@mgree mgree marked this pull request as ready for review February 9, 2026 20:34
@mgree mgree requested review from a team as code owners February 9, 2026 20:34
@mgree mgree requested a review from ggevay February 9, 2026 20:34
@ggevay
Copy link
Contributor

ggevay commented Feb 9, 2026

For the test fails:

  • 💡 SQL logic tests (4 replicas) 4: You could put your file into tests_without_views_and_replica, so that it's not run with multiple replicas.
  • 🏎️ testdrive 4 replicas: Just a rare flake, I've hit retry.
  • Orchestratord + upgrade, combine props: common flake

&& e1
.typ(column_types)
.union(&e2.typ(column_types))
.sql_union(&e2.typ(column_types))
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@mgree mgree force-pushed the repr-types-check-with-fallback branch from 3235085 to 3785218 Compare February 10, 2026 19:10
@mgree mgree enabled auto-merge (squash) February 10, 2026 19:11
@mgree mgree merged commit 201dcae into MaterializeInc:main Feb 10, 2026
133 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants