adapter: better error message for replacement schema mismatch #34670
Conversation
bkirwi
left a comment
bkirwi
left a comment
There was a problem hiding this comment.
Overall +1 from me! Commented on a couple of things to look at before merging.
src/repr/src/relation.rs
Outdated
| /// Keys of the left relation. | ||
| pub left: Vec<Vec<ColumnName>>, | ||
| /// Keys of the right relation. | ||
| pub right: Vec<Vec<ColumnName>>, |
There was a problem hiding this comment.
Is the order of keys meaningful? (If we have the same keys in different orders, would this be unnecessarily strict?)
There was a problem hiding this comment.
Probably not, at least not in the rich diff. Might not even be meaningful in the RelationDesc type. We use the first element as the "default key" for default indexes, but I don't think we really care which key we use for that. I'm also not sure under what conditions we'd end up with multiple keys.
There was a problem hiding this comment.
I made these BTreeSets in the diff type but didn't touch RelationDesc itself, as that seems like it would require a larger refactor.
I also realized that testing for schema changes using PartialEq is not correct because it wouldn't treat two schemas with the same keys in different orders as not equal. So in the replacement MV sequencing I'm now always computing the diff and then checking if that is empty.
| let right_arity = other.arity(); | ||
| let common_arity = std::cmp::min(left_arity, right_arity); | ||
|
|
||
| for idx in 0..common_arity { |
There was a problem hiding this comment.
I don't think RelationDesc actually guarantees that these are dense... if eg. drop_column has been called.
It's probably accurate for now, though, since nobody currently calls those methods! But ideally we'd either leave a comment / warning here or iterate over the indices that are actually present.
There was a problem hiding this comment.
Hm, interesting! I can't quite wrap my head around what it means to have holes in the column indexes. There can't be any holes in Rows right?
For example, suppose I have a RelationDesc with three columns (a int, b int, c int), but b has been dropped. How do rows in that relation look like at the SQL level? (1, NULL, 3)? (1, 3)? If it's the latter, does that mean that column indexes don't directly correspond to positions in the output and the error/diff reporting has to take that into account?
This commit adds much more detail to the error message users see when they try to create a replacement materialized view with a schema different from the target. Some of the mismatches (nullability, keys) can be surprising, so good error reporting is essential to avoid user confusion. Previously users would see the error: ``` ERROR: incompatible schema ``` Now they get detailed information about the mismatch: ``` ERROR: replacement schema differs from target schema DETAIL: column at position 1: name mismatch (target: "a", replacement: "c") ```
teskje
force-pushed
the
replacement-mv-schema-mismatch-error
branch
from
a8590e3 to
b13aa38
Compare
2 participants
This PR adds much more detail to the error message users see when they try to create a replacement materialized view with a schema different from the target. Some of the mismatches (nullability, keys) can be surprising, so good error reporting is essential to avoid user confusion.
Previously users would see the error:
Now they get detailed information about the mismatch:
Motivation
Part of https://github.com/MaterializeInc/database-issues/issues/9903
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.