Fix EXPLAIN around Char and Varchar by ggevay · Pull Request #31533 · MaterializeInc/materialize

ggevay · 2025-02-18T15:57:35Z

Char and Varchar can involve a length. Additionally, the cast functions text_to_char and text_to_varchar involve a fail_on_len option. We were not printing these extra fields in EXPLAIN and in error msgs, causing confusion in various situations, e.g.,
https://github.com/MaterializeInc/database-issues/issues/8807#issuecomment-2518662460
https://github.com/MaterializeInc/database-issues/issues/1291
#27029

The first commit attends to text_to_varchar and text_to_char.

The second commit attends to the type names, changing them in EXPLAIN, error msgs, and pg_typeof.

The third commit reverts the change for pg_typeof, to keep Postgres compatibility.

Nightly: https://buildkite.com/materialize/nightly/builds/11207

Motivation

This PR fixes a recognized bug: https://github.com/MaterializeInc/database-issues/issues/8975

Tips for reviewer

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.

ggevay · 2025-02-18T17:09:14Z

There is a problem: The output of pg_typeof is changing with this PR, e.g. from

character varying

to

varchar(1)

This might be a problem, because maybe some external tools rely on these being exactly what Postgres says. In that case, I'll have to separate the pg_typeof code path from the EXPLAIN code path. I'll ask the SQL council.

ggevay · 2025-02-18T17:29:33Z

Will tweak the PR to not change pg_typeof. Council discussion.

mgree

Looks good to me. I like the new, clearer output. There's some risk of using non postgres_compat output in error messages leading people to get confused about type names. Not a huge risk, likely better this way.

mgree · 2025-02-19T17:19:57Z

src/expr/src/scalar/func/impls/string.rs

+                    self.fail_on_len
+                )
+            }
+            None => f.write_str("text_to_char[len=None]"),


The None is a Rust-ism:

Suggested change

None => f.write_str("text_to_char[len=None]"),

None => f.write_str("text_to_char[len=unbounded]"),

Thanks, unbounded is indeed better!

src/expr/src/scalar/func/impls/string.rs

mgree · 2025-02-19T17:23:11Z

src/sql/src/func.rs

                    Ok(elem_type) => elem_type,
                    Err(elem_type) => bail_unsupported!(
-                        format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type))
+                        format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type, false))


Suggested change

format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type, false))

// This will be used in error msgs, therefore we call with `postgres_compat` false.

format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type, false))

... and probably similar at other uses.

Changed this one. Generally, I didn't add this comment to all call sites, only to those where it was not immediately clear whether it's only used in an error msg.

mgree · 2025-02-19T17:27:41Z

test/sqllogictest/char.slt

 # Fixes database-issues#5222

-query error db error: ERROR: coalesce could not convert type "char" to character
+query error db error: ERROR: coalesce could not convert type "char" to char


Oof, tough error message!

Yeah, life is just harsh! :D

But one thing we could possibly do here is to print it more verbosely when postgres_compat is off. For example, we could print it as "char" aka PgLegacyChar. See this comment. What do you think @mgree ?

Sorry to miss this. I think we can let it be confusing for now.

Ok. We can do it in a follow-up PR if we think so later.

test/sqllogictest/github-5536.slt

test/testdrive/char-varchar-distinct.td

SQL council discussion with Marta: https://materializeinc.slack.com/archives/C063H5S7NKE/p1739899659699649?thread_ts=1739898930.961779&cid=C063H5S7NKE

ggevay · 2025-02-20T11:21:56Z

Thanks for the review! Merging.

If we decide to do something about the could not convert type "char" to char error msg, then I'll open a follow-up PR for that.

(The Nightly failure is unrelated.)

ggevay requested review from a team as code owners February 18, 2025 15:57

ggevay requested a review from ParkMyCar February 18, 2025 15:57

ggevay added the A-CLUSTER Topics related to the CLUSTER layer label Feb 18, 2025

ggevay changed the title ~~Fix EXPLAIN around Char Varchar~~ Fix EXPLAIN around Char and Varchar Feb 18, 2025

ggevay marked this pull request as draft February 18, 2025 17:09

ggevay force-pushed the char-varchar-printing branch 3 times, most recently from 3defdae to a846d48 Compare February 19, 2025 13:10

ggevay marked this pull request as ready for review February 19, 2025 13:53

ggevay requested a review from mgree February 19, 2025 14:59

mgree approved these changes Feb 19, 2025

View reviewed changes

ggevay added 4 commits February 19, 2025 19:11

Fix text_to_varchar and text_to_char EXPLAIN printing

8f53e03

Fix char and varchar typename printing in EXPLAIN and error msgs

4a24290

Revert char/varchar printing in pg_typeof

1b69c4c

SQL council discussion with Marta: https://materializeinc.slack.com/archives/C063H5S7NKE/p1739899659699649?thread_ts=1739898930.961779&cid=C063H5S7NKE

Print char/varchar None length as unbounded

377a61e

ggevay force-pushed the char-varchar-printing branch from a846d48 to 377a61e Compare February 20, 2025 11:19

ggevay enabled auto-merge February 20, 2025 11:21

ggevay merged commit c962d78 into MaterializeInc:main Feb 20, 2025
80 checks passed

	None => f.write_str("text_to_char[len=None]"),
	None => f.write_str("text_to_char[len=unbounded]"),

	format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type, false))
	// This will be used in error msgs, therefore we call with `postgres_compat` false.
	format!("array_fill on {}", ecx.humanize_scalar_type(&elem_type, false))

Conversation

ggevay commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Tips for reviewer

Checklist

Uh oh!

ggevay commented Feb 18, 2025

Uh oh!

ggevay commented Feb 18, 2025

Uh oh!

mgree left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ggevay commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ggevay commented Feb 18, 2025 •

edited

Loading

ggevay commented Feb 20, 2025 •

edited

Loading