Skip to content

Commit a846d48

Browse files
ggevayggevay
committed
Revert char/varchar printing in pg_typeof
SQL council discussion with Marta: https://materializeinc.slack.com/archives/C063H5S7NKE/p1739899659699649?thread_ts=1739898930.961779&cid=C063H5S7NKE
1 parent bf269ed commit a846d48

File tree

14 files changed

+109
-78
lines changed

14 files changed

+109
-78
lines changed

‎src/adapter/src/catalog.rs‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1633,11 +1633,11 @@ impl ExprHumanizer for ConnCatalog<'_> {
16331633
Some(self.resolve_full_name(entry.name()).into_parts())
16341634
}
16351635

1636-
fn humanize_scalar_type(&self, typ: &ScalarType) -> String {
1636+
fn humanize_scalar_type(&self, typ: &ScalarType, postgres_compat: bool) -> String {
16371637
use ScalarType::*;
16381638

16391639
match typ {
1640-
Array(t) => format!("{}[]", self.humanize_scalar_type(t)),
1640+
Array(t) => format!("{}[]", self.humanize_scalar_type(t, postgres_compat)),
16411641
List {
16421642
custom_id: Some(item_id),
16431643
..
@@ -1650,12 +1650,15 @@ impl ExprHumanizer for ConnCatalog<'_> {
16501650
self.minimal_qualification(item.name()).to_string()
16511651
}
16521652
List { element_type, .. } => {
1653-
format!("{} list", self.humanize_scalar_type(element_type))
1653+
format!(
1654+
"{} list",
1655+
self.humanize_scalar_type(element_type, postgres_compat)
1656+
)
16541657
}
16551658
Map { value_type, .. } => format!(
16561659
"map[{}=>{}]",
1657-
self.humanize_scalar_type(&ScalarType::String),
1658-
self.humanize_scalar_type(value_type)
1660+
self.humanize_scalar_type(&ScalarType::String, postgres_compat),
1661+
self.humanize_scalar_type(value_type, postgres_compat)
16591662
),
16601663
Record {
16611664
custom_id: Some(item_id),
@@ -1668,15 +1671,19 @@ impl ExprHumanizer for ConnCatalog<'_> {
16681671
"record({})",
16691672
fields
16701673
.iter()
1671-
.map(|f| format!("{}: {}", f.0, self.humanize_column_type(&f.1)))
1674+
.map(|f| format!(
1675+
"{}: {}",
1676+
f.0,
1677+
self.humanize_column_type(&f.1, postgres_compat)
1678+
))
16721679
.join(",")
16731680
),
16741681
PgLegacyChar => "\"char\"".into(),
1675-
Char { length } => match length {
1682+
Char { length } if !postgres_compat => match length {
16761683
None => "char".into(),
16771684
Some(length) => format!("char({})", length.into_u32()),
16781685
},
1679-
VarChar { max_length } => match max_length {
1686+
VarChar { max_length } if !postgres_compat => match max_length {
16801687
None => "varchar".into(),
16811688
Some(length) => format!("varchar({})", length.into_u32()),
16821689
},

‎src/adapter/src/coord/timestamp_selection.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,9 @@ impl Coordinator {
647647
.try_into()?,
648648
_ => coord_bail!(
649649
"can't use {} as a mz_timestamp for AS OF or UP TO",
650-
catalog.for_session(session).humanize_column_type(&ty)
650+
catalog
651+
.for_session(session)
652+
.humanize_column_type(&ty, false)
651653
),
652654
})
653655
}

‎src/compute-types/src/explain/text.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -458,7 +458,9 @@ impl DisplayText<PlanRenderingContext<'_, Plan>> for AvailableCollections {
458458
"{}",
459459
separated(
460460
", ",
461-
types.iter().map(|c| ctx.humanizer.humanize_column_type(c))
461+
types
462+
.iter()
463+
.map(|c| ctx.humanizer.humanize_column_type(c, false))
462464
)
463465
)?;
464466
writeln!(f, "]")?;

‎src/expr-parser/src/catalog.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,8 @@ impl ExprHumanizer for TestCatalog {
8383
self.humanize_id_unqualified(id).map(|name| vec![name])
8484
}
8585

86-
fn humanize_scalar_type(&self, ty: &ScalarType) -> String {
87-
DummyHumanizer.humanize_scalar_type(ty)
86+
fn humanize_scalar_type(&self, ty: &ScalarType, postgres_compat: bool) -> String {
87+
DummyHumanizer.humanize_scalar_type(ty, postgres_compat)
8888
}
8989

9090
fn column_names_for_id(&self, id: GlobalId) -> Option<Vec<String>> {

‎src/expr-test-util/src/lib.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,8 +182,8 @@ impl ExprHumanizer for TestCatalog {
182182
self.humanize_id_unqualified(id).map(|name| vec![name])
183183
}
184184

185-
fn humanize_scalar_type(&self, ty: &ScalarType) -> String {
186-
DummyHumanizer.humanize_scalar_type(ty)
185+
fn humanize_scalar_type(&self, ty: &ScalarType, postgres_compat: bool) -> String {
186+
DummyHumanizer.humanize_scalar_type(ty, postgres_compat)
187187
}
188188

189189
fn column_names_for_id(&self, _id: GlobalId) -> Option<Vec<String>> {

‎src/repr/src/explain.rs‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -438,15 +438,19 @@ pub trait ExprHumanizer: fmt::Debug {
438438
fn humanize_id_parts(&self, id: GlobalId) -> Option<Vec<String>>;
439439

440440
/// Returns a human-readable name for the specified scalar type.
441-
/// Used in, e.g., EXPLAIN and error msgs.
442-
fn humanize_scalar_type(&self, ty: &ScalarType) -> String;
441+
/// Used in, e.g., EXPLAIN and error msgs, in which case exact Postgres compatibility is less
442+
/// important than showing as much detail as possible. Also used in `pg_typeof`, where Postgres
443+
/// compatibility is more important.
444+
fn humanize_scalar_type(&self, ty: &ScalarType, postgres_compat: bool) -> String;
443445

444446
/// Returns a human-readable name for the specified column type.
445-
/// Used in, e.g., EXPLAIN and error msgs.
446-
fn humanize_column_type(&self, typ: &ColumnType) -> String {
447+
/// Used in, e.g., EXPLAIN and error msgs, in which case exact Postgres compatibility is less
448+
/// important than showing as much detail as possible. Also used in `pg_typeof`, where Postgres
449+
/// compatibility is more important.
450+
fn humanize_column_type(&self, typ: &ColumnType, postgres_compat: bool) -> String {
447451
format!(
448452
"{}{}",
449-
self.humanize_scalar_type(&typ.scalar_type),
453+
self.humanize_scalar_type(&typ.scalar_type, postgres_compat),
450454
if typ.nullable { "?" } else { "" }
451455
)
452456
}
@@ -507,8 +511,8 @@ impl<'a> ExprHumanizer for ExprHumanizerExt<'a> {
507511
}
508512
}
509513

510-
fn humanize_scalar_type(&self, ty: &ScalarType) -> String {
511-
self.inner.humanize_scalar_type(ty)
514+
fn humanize_scalar_type(&self, ty: &ScalarType, postgres_compat: bool) -> String {
515+
self.inner.humanize_scalar_type(ty, postgres_compat)
512516
}
513517

514518
fn column_names_for_id(&self, id: GlobalId) -> Option<Vec<String>> {
@@ -574,7 +578,7 @@ impl ExprHumanizer for DummyHumanizer {
574578
None
575579
}
576580

577-
fn humanize_scalar_type(&self, ty: &ScalarType) -> String {
581+
fn humanize_scalar_type(&self, ty: &ScalarType, _postgres_compat: bool) -> String {
578582
// The debug implementation is better than nothing.
579583
format!("{:?}", ty)
580584
}
@@ -685,7 +689,7 @@ impl<'a> Display for HumanizedAnalyses<'a> {
685689
Some(types) => {
686690
let types = types
687691
.into_iter()
688-
.map(|c| self.humanizer.humanize_column_type(c))
692+
.map(|c| self.humanizer.humanize_column_type(c, false))
689693
.collect::<Vec<_>>();
690694

691695
bracketed("(", ")", separated(", ", types)).to_string()

‎src/sql/src/func.rs‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,7 +1009,8 @@ where
10091009
let arg_types: Vec<_> = types
10101010
.into_iter()
10111011
.map(|ty| match ty {
1012-
CoercibleScalarType::Coerced(ty) => ecx.humanize_scalar_type(&ty),
1012+
// This will be used in error msgs, therefore we call with `postgres_compat` false.
1013+
CoercibleScalarType::Coerced(ty) => ecx.humanize_scalar_type(&ty, false),
10131014
CoercibleScalarType::Record(_) => "record".to_string(),
10141015
CoercibleScalarType::Uncoerced => "unknown".to_string(),
10151016
})
@@ -1809,7 +1810,7 @@ pub static PG_CATALOG_BUILTINS: LazyLock<BTreeMap<&'static str, Func>> = LazyLoc
18091810
let elem_type = match elem_type.array_of_self_elem_type() {
18101811
Ok(elem_type) => elem_type,
18111812
Err(elem_type) => bail_unsupported!(
1812-
format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type))
1813+
format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type, false))
18131814
),
18141815
};
18151816

@@ -1825,7 +1826,7 @@ pub static PG_CATALOG_BUILTINS: LazyLock<BTreeMap<&'static str, Func>> = LazyLoc
18251826
let elem_type = match elem_type.array_of_self_elem_type() {
18261827
Ok(elem_type) => elem_type,
18271828
Err(elem_type) => bail_unsupported!(
1828-
format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type))
1829+
format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type, false))
18291830
),
18301831
};
18311832

@@ -2520,7 +2521,7 @@ pub static PG_CATALOG_BUILTINS: LazyLock<BTreeMap<&'static str, Func>> = LazyLoc
25202521
let name = match ecx.scalar_type(&exprs[0]) {
25212522
CoercibleScalarType::Uncoerced => "unknown".to_string(),
25222523
CoercibleScalarType::Record(_) => "record".to_string(),
2523-
CoercibleScalarType::Coerced(ty) => ecx.humanize_scalar_type(&ty),
2524+
CoercibleScalarType::Coerced(ty) => ecx.humanize_scalar_type(&ty, true),
25242525
};
25252526

25262527
// For consistency with other functions, verify that
@@ -3023,7 +3024,7 @@ pub static PG_CATALOG_BUILTINS: LazyLock<BTreeMap<&'static str, Func>> = LazyLoc
30233024
let elem_type = match elem_type.array_of_self_elem_type() {
30243025
Ok(elem_type) => elem_type,
30253026
Err(elem_type) => bail_unsupported!(
3026-
format!("array_agg on {}", ecx.humanize_scalar_type(&elem_type))
3027+
format!("array_agg on {}", ecx.humanize_scalar_type(&elem_type, false))
30273028
),
30283029
};
30293030

@@ -3759,7 +3760,7 @@ pub static MZ_CATALOG_BUILTINS: LazyLock<BTreeMap<&'static str, Func>> = LazyLoc
37593760
let err = || {
37603761
Err(sql_err!(
37613762
"function map_build({}) does not exist",
3762-
ecx.humanize_scalar_type(&ty.clone())
3763+
ecx.humanize_scalar_type(&ty.clone(), false)
37633764
))
37643765
};
37653766

‎src/sql/src/plan/hir.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -834,8 +834,8 @@ impl CoercibleScalarExpr {
834834
sql_bail!(
835835
"{} must have type {}, not type {}",
836836
ecx.name,
837-
ecx.humanize_scalar_type(ty),
838-
ecx.humanize_scalar_type(&expr_ty),
837+
ecx.humanize_scalar_type(ty, false),
838+
ecx.humanize_scalar_type(&expr_ty, false),
839839
);
840840
}
841841
Ok(expr)

0 commit comments

Comments
 (0)