Skip to content

Commit 3defdae

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 3defdae

File tree

12 files changed

+105
-74
lines changed

12 files changed

+105
-74
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/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)

‎src/sql/src/plan/query.rs‎

Lines changed: 40 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -391,8 +391,8 @@ pub fn plan_insert_query(
391391
sql_err!(
392392
"column {} is of type {} but expression is of type {}",
393393
desc.get_name(ordering[e.column]).as_str().quoted(),
394-
qcx.humanize_scalar_type(&e.target_type),
395-
qcx.humanize_scalar_type(&e.source_type),
394+
qcx.humanize_scalar_type(&e.target_type, false),
395+
qcx.humanize_scalar_type(&e.source_type, false),
396396
)
397397
})?;
398398

@@ -1255,8 +1255,8 @@ pub fn plan_params<'a>(
12551255
if st != *ty {
12561256
sql_bail!(
12571257
"mismatched parameter type: expected {}, got {}",
1258-
ecx.humanize_scalar_type(ty),
1259-
ecx.humanize_scalar_type(&st),
1258+
ecx.humanize_scalar_type(ty, false),
1259+
ecx.humanize_scalar_type(&st, false),
12601260
);
12611261
}
12621262
let ex = ex.lower_uncorrelated()?;
@@ -1602,12 +1602,12 @@ pub fn plan_ctes(
16021602
let proposed_typ = proposed_typ
16031603
.column_types
16041604
.iter()
1605-
.map(|ty| qcx.humanize_scalar_type(&ty.scalar_type))
1605+
.map(|ty| qcx.humanize_scalar_type(&ty.scalar_type, false))
16061606
.collect::<Vec<_>>();
16071607
let inferred_typ = derived_typ
16081608
.column_types
16091609
.iter()
1610-
.map(|ty| qcx.humanize_scalar_type(&ty.scalar_type))
1610+
.map(|ty| qcx.humanize_scalar_type(&ty.scalar_type, false))
16111611
.collect::<Vec<_>>();
16121612
Err(PlanError::RecursiveTypeMismatch(
16131613
cte_name,
@@ -1620,7 +1620,7 @@ pub fn plan_ctes(
16201620
return type_err(proposed_typ, derived_typ);
16211621
}
16221622

1623-
// Cast dervied types to proposed types or error.
1623+
// Cast derived types to proposed types or error.
16241624
let val = match cast_relation(
16251625
qcx,
16261626
// Choose `CastContext::Assignment`` because the user has
@@ -1753,8 +1753,8 @@ fn plan_set_expr(
17531753
Err(_) => sql_bail!(
17541754
"{} types {} and {} cannot be matched",
17551755
op,
1756-
qcx.humanize_scalar_type(&left_type.scalar_type),
1757-
qcx.humanize_scalar_type(&target),
1756+
qcx.humanize_scalar_type(&left_type.scalar_type, false),
1757+
qcx.humanize_scalar_type(&target, false),
17581758
),
17591759
}
17601760
match typeconv::plan_cast(
@@ -1767,8 +1767,8 @@ fn plan_set_expr(
17671767
Err(_) => sql_bail!(
17681768
"{} types {} and {} cannot be matched",
17691769
op,
1770-
qcx.humanize_scalar_type(&target),
1771-
qcx.humanize_scalar_type(&right_type.scalar_type),
1770+
qcx.humanize_scalar_type(&target, false),
1771+
qcx.humanize_scalar_type(&right_type.scalar_type, false),
17721772
),
17731773
}
17741774
}
@@ -2034,8 +2034,8 @@ fn plan_values_insert(
20342034
Err(_) => sql_bail!(
20352035
"column {} is of type {} but expression is of type {}",
20362036
target_names[column].as_str().quoted(),
2037-
qcx.humanize_scalar_type(target_type),
2038-
qcx.humanize_scalar_type(source_type),
2037+
qcx.humanize_scalar_type(target_type, false),
2038+
qcx.humanize_scalar_type(source_type, false),
20392039
),
20402040
};
20412041
if column >= types.len() {
@@ -3418,7 +3418,10 @@ fn expand_select_item<'a>(
34183418
let expr = plan_expr(ecx, sql_expr)?.type_as_any(ecx)?;
34193419
let fields = match ecx.scalar_type(&expr) {
34203420
ScalarType::Record { fields, .. } => fields,
3421-
ty => sql_bail!("type {} is not composite", ecx.humanize_scalar_type(&ty)),
3421+
ty => sql_bail!(
3422+
"type {} is not composite",
3423+
ecx.humanize_scalar_type(&ty, false)
3424+
),
34223425
};
34233426
let mut skip_cols: BTreeSet<ColumnName> = BTreeSet::new();
34243427
if let Expr::Identifier(ident) = sql_expr.as_ref() {
@@ -3998,14 +4001,14 @@ fn plan_field_access(
39984001
ScalarType::Record { fields, .. } => fields.iter().position(|(name, _ty)| *name == field),
39994002
ty => sql_bail!(
40004003
"column notation applied to type {}, which is not a composite type",
4001-
ecx.humanize_scalar_type(ty)
4004+
ecx.humanize_scalar_type(ty, false)
40024005
),
40034006
};
40044007
match i {
40054008
None => sql_bail!(
40064009
"field {} not found in data type {}",
40074010
field,
4008-
ecx.humanize_scalar_type(&ty)
4011+
ecx.humanize_scalar_type(&ty, false)
40094012
),
40104013
Some(i) => Ok(expr
40114014
.call_unary(UnaryFunc::RecordGet(expr_func::RecordGet(i)))
@@ -4038,11 +4041,15 @@ fn plan_subscript(
40384041
),
40394042
ScalarType::Jsonb => plan_subscript_jsonb(ecx, expr, positions),
40404043
ScalarType::List { element_type, .. } => {
4041-
let elem_type_name = ecx.humanize_scalar_type(element_type);
4044+
// `elem_type_name` is used only in error msgs, so we set `postgres_compat` to false.
4045+
let elem_type_name = ecx.humanize_scalar_type(element_type, false);
40424046
let n_layers = ty.unwrap_list_n_layers();
40434047
plan_subscript_list(ecx, expr, positions, n_layers, &elem_type_name)
40444048
}
4045-
ty => sql_bail!("cannot subscript type {}", ecx.humanize_scalar_type(ty)),
4049+
ty => sql_bail!(
4050+
"cannot subscript type {}",
4051+
ecx.humanize_scalar_type(ty, false)
4052+
),
40464053
}
40474054
}
40484055

@@ -4416,7 +4423,7 @@ where
44164423
if is_unsupported_type(&elem_type) {
44174424
bail_unsupported!(format!(
44184425
"cannot build array from subquery because return type {}{}",
4419-
ecx.humanize_scalar_type(&elem_type),
4426+
ecx.humanize_scalar_type(&elem_type, false),
44204427
vector_type_string
44214428
));
44224429
}
@@ -4672,7 +4679,7 @@ fn plan_array(
46724679
elem_type,
46734680
ScalarType::Char { .. } | ScalarType::List { .. } | ScalarType::Map { .. }
46744681
) {
4675-
bail_unsupported!(format!("{}[]", ecx.humanize_scalar_type(&elem_type)));
4682+
bail_unsupported!(format!("{}[]", ecx.humanize_scalar_type(&elem_type, false)));
46764683
}
46774684

46784685
Ok(HirScalarExpr::CallVariadic {
@@ -4816,8 +4823,8 @@ pub fn coerce_homogeneous_exprs(
48164823
Err(_) => sql_bail!(
48174824
"{} could not convert type {} to {}",
48184825
ecx.name,
4819-
ecx.humanize_scalar_type(&ecx.scalar_type(&arg)),
4820-
ecx.humanize_scalar_type(target),
4826+
ecx.humanize_scalar_type(&ecx.scalar_type(&arg), false),
4827+
ecx.humanize_scalar_type(target, false),
48214828
),
48224829
}
48234830
}
@@ -5334,7 +5341,7 @@ pub fn resolve_func(
53345341
let arg_types: Vec<_> = cexprs
53355342
.into_iter()
53365343
.map(|ty| match ecx.scalar_type(&ty) {
5337-
CoercibleScalarType::Coerced(ty) => ecx.humanize_scalar_type(&ty),
5344+
CoercibleScalarType::Coerced(ty) => ecx.humanize_scalar_type(&ty, false),
53385345
CoercibleScalarType::Record(_) => "record".to_string(),
53395346
CoercibleScalarType::Uncoerced => "unknown".to_string(),
53405347
})
@@ -5713,8 +5720,8 @@ pub fn scalar_type_from_sql(
57135720
ScalarType::String => {}
57145721
other => sql_bail!(
57155722
"map key type must be {}, got {}",
5716-
scx.humanize_scalar_type(&ScalarType::String),
5717-
scx.humanize_scalar_type(&other)
5723+
scx.humanize_scalar_type(&ScalarType::String, false),
5724+
scx.humanize_scalar_type(&other, false)
57185725
),
57195726
}
57205727
Ok(ScalarType::Map {
@@ -6321,8 +6328,10 @@ impl<'a> QueryContext<'a> {
63216328
}
63226329
}
63236330

6324-
pub fn humanize_scalar_type(&self, typ: &ScalarType) -> String {
6325-
self.scx.humanize_scalar_type(typ)
6331+
/// The returned String is more detailed when the `postgres_compat` flag is not set. However,
6332+
/// the flag should be set in, e.g., the implementation of the `pg_typeof` function.
6333+
pub fn humanize_scalar_type(&self, typ: &ScalarType, postgres_compat: bool) -> String {
6334+
self.scx.humanize_scalar_type(typ, postgres_compat)
63266335
}
63276336
}
63286337

@@ -6397,7 +6406,9 @@ impl<'a> ExprContext<'a> {
63976406
&self.qcx.scx.param_types
63986407
}
63996408

6400-
pub fn humanize_scalar_type(&self, typ: &ScalarType) -> String {
6401-
self.qcx.scx.humanize_scalar_type(typ)
6409+
/// The returned String is more detailed when the `postgres_compat` flag is not set. However,
6410+
/// the flag should be set in, e.g., the implementation of the `pg_typeof` function.
6411+
pub fn humanize_scalar_type(&self, typ: &ScalarType, postgres_compat: bool) -> String {
6412+
self.qcx.scx.humanize_scalar_type(typ, postgres_compat)
64026413
}
64036414
}

0 commit comments

Comments
 (0)