Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: fix BPCHAR type and CASE typing #129007

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Aug 14, 2024

This commit addresses inconsistencies from Postgres' behavior. First, it
makes the BPCHAR type distinct from CHAR. The former is a
blank-padded character type with no type width, meaning that it could
have any length. The latter is a blank-padded character type with a type
width of exactly 1 - it is essentially an alias of CHAR(1).
Previously, a column of type BPCHAR behaved the same as a column of
type CHAR(1) - it enforced a length limit of 1.

Second, the typing of CASE and CASE-like expressions has been fixed.
The branches of these conditionals is no longer forced to have the same
type-width.

Fixes #127889
Fixes #108360

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of CASE, COALESCE, and IF expressions with branches
producing fixed-width string-like types, such as CHAR. In addition,
the BPCHAR type has been fixed so that it no longer incorrectly
imposes a length limit of 1.

@mgartner mgartner added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Aug 14, 2024
@mgartner mgartner requested a review from a team as a code owner August 14, 2024 20:18
@mgartner mgartner requested review from DrewKimball and removed request for a team August 14, 2024 20:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner requested a review from a team August 14, 2024 20:19
Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! Assuming the test cases check out, :lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)


pkg/sql/logictest/testdata/logic_test/case line 71 at r1 (raw file):

ba

subtest end

I'd be curious to see a few additional tests:

SELECT CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END;
SELECT (CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END)::CHAR;
SELECT (CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END)::BPCHAR;

In Postgres:

postgres=# SELECT CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END;
SELECT (CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END)::CHAR;
SELECT (CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END)::BPCHAR;
 case
------
 foo
(1 row)

 bpchar
--------
 f
(1 row)

 bpchar
--------
 foo
(1 row)

@mgartner mgartner force-pushed the 127889-fix-case-typing branch from 8a0a160 to 6c9f9e8 Compare August 15, 2024 13:37
Copy link
Collaborator Author

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball)


pkg/sql/logictest/testdata/logic_test/case line 71 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I'd be curious to see a few additional tests:

SELECT CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END;
SELECT (CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END)::CHAR;
SELECT (CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END)::BPCHAR;

In Postgres:

postgres=# SELECT CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END;
SELECT (CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END)::CHAR;
SELECT (CASE WHEN false THEN 'b'::CHAR ELSE 'foo'::TEXT END)::BPCHAR;
 case
------
 foo
(1 row)

 bpchar
--------
 f
(1 row)

 bpchar
--------
 foo
(1 row)

Done.

@mgartner mgartner force-pushed the 127889-fix-case-typing branch from 6c9f9e8 to 2860e1c Compare August 16, 2024 19:58
@mgartner
Copy link
Collaborator Author

I realized this is related to #108360. I'll have a follow-up PR to address a few more things I found (mentioned in that issue).

@mgartner
Copy link
Collaborator Author

I actually need to do the follow-up work in this same PR to get the tests to pass. We need to represent a CHAR(N) type with no type width in SQL when we serialize the cast for distributing it to other nodes. Because we simply serialize it as CHAR now, the remote node parses it as CHAR(1) semantically. This causes the typing logic tests to fail under fakedist and other configurations.

@mgartner mgartner removed backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Aug 21, 2024
@mgartner mgartner force-pushed the 127889-fix-case-typing branch from 2860e1c to d310e8a Compare August 21, 2024 19:50
@mgartner mgartner requested review from a team as code owners August 21, 2024 19:50
@mgartner mgartner added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Aug 21, 2024
@mgartner mgartner changed the title opt: do not impose type width on CASE branches opt: fix BPCHAR type and CASE typing Aug 21, 2024
@mgartner mgartner force-pushed the 127889-fix-case-typing branch 3 times, most recently from 9913264 to 35086d0 Compare August 28, 2024 19:54
@mgartner mgartner force-pushed the 127889-fix-case-typing branch 2 times, most recently from 3867c68 to 7db5e7c Compare August 30, 2024 23:10
@mgartner mgartner force-pushed the 127889-fix-case-typing branch from 7db5e7c to 98e33e2 Compare September 4, 2024 21:53
@mgartner
Copy link
Collaborator Author

mgartner commented Sep 4, 2024

I had to adjust some of the UDF resolution logic to find overloads with CHAR, CHAR(N), or BPCHAR parameters with any three of those variations. The behavior now matches Postgres more closely.

This commit addresses inconsistencies from Postgres' behavior. First, it
makes the `BPCHAR` type distinct from `CHAR`. The former is a
blank-padded character type with no type width, meaning that it could
have any length. The latter is a blank-padded character type with a type
width of exactly 1 - it is essentially an alias of `CHAR(1)`.
Previously, a column of type `BPCHAR` behaved the same as a column of
type `CHAR(1)` - it enforced a length limit of 1.

Second, the typing of `CASE` and `CASE`-like expressions has been fixed.
The branches of these conditionals is no longer forced to have the same
type-width.

Fixes cockroachdb#127889
Fixes cockroachdb#108360

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of `CASE`, `COALESCE`, and `IF` expressions with branches
producing fixed-width string-like types, such as `CHAR`. In addition,
the `BPCHAR` type has been fixed so that it no longer incorrectly
imposes a length limit of 1.
@mgartner mgartner force-pushed the 127889-fix-case-typing branch from 98e33e2 to 4a11c7d Compare September 5, 2024 13:25
@mgartner
Copy link
Collaborator Author

mgartner commented Sep 5, 2024

@DrewKimball mind taking one more quick look?

@mgartner
Copy link
Collaborator Author

Friendly ping @DrewKimball. :)

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r2, 10 of 22 files at r3, 16 of 16 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

@mgartner
Copy link
Collaborator Author

TFTR!

bors r+

Copy link

blathers-crl bot commented Sep 13, 2024

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.


Issue #127889: branch-release-23.1.


Issue #108360: branch-release-23.1, branch-release-23.2, branch-release-24.1, branch-release-24.2.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Copy link

blathers-crl bot commented Sep 13, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 4a11c7d to blathers/backport-release-23.1-129007: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


error creating merge commit from 4a11c7d to blathers/backport-release-23.2-129007: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.


error creating merge commit from 4a11c7d to blathers/backport-release-24.1-129007: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.1.x failed. See errors above.


error creating merge commit from 4a11c7d to blathers/backport-release-24.2-129007: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@mgartner mgartner deleted the 127889-fix-case-typing branch September 16, 2024 15:00
mgartner added a commit to mgartner/cockroach that referenced this pull request Oct 11, 2024
A TODO that was addressed in cockroachdb#129007 has been removed.

Release note: None
craig bot pushed a commit that referenced this pull request Oct 14, 2024
132338: pgwire: reduce allocations when writing CHAR(N) datums r=mgartner a=mgartner

`ResolveBlankPaddedCharBytes` has been replaced with a more efficient
method of padding `CHAR(N)` datums with trailing spaces.

Epic: None

Release note: None


132501: opt: remove outdated  TODO r=mgartner a=mgartner

A TODO that was addressed in #129007 has been removed.

Release note: None

132563: kvserver: skip empty RACv1 dispatches r=kvoli a=pav-kv

This commit skips creating a `RaftMessageRequest` in the fallback RAC admission dispatch code if there are no pending RACv1 dispatches. Previously, it would send an empty `RaftMessageRequest` which would cause an error in the receiver's `handleRaftRequest`, [message drops](https://github.com/cockroachdb/cockroach/blob/9e12f67ff4ad860651c40dcef489a1556d1d11b7/pkg/kv/kvserver/raft_transport.go#L451-L456), and the receiver's message queue to restart after the following warning:

```
unable to accept Raft message from (n0,s0):?: no handler registered for (n0,s0):?
```

Related to #129508

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>
mw5h pushed a commit to mw5h/cockroach that referenced this pull request Oct 14, 2024
A TODO that was addressed in cockroachdb#129007 has been removed.

Release note: None
@mgartner mgartner removed the backport-23.1.x Flags PRs that need to be backported to 23.1 label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2
Projects
None yet
3 participants