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

release-23.1: opt: fix BPCHAR type and CASE typing #130899

Closed

Conversation

mgartner
Copy link
Collaborator

Backport 1/1 commits from #129007.

/cc @cockroachdb/release


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.


Release justification: Low-risk bug fix.

@mgartner mgartner requested review from a team as code owners September 17, 2024 19:34
@mgartner mgartner requested review from kev-cao and removed request for a team September 17, 2024 19:34
Copy link

blathers-crl bot commented Sep 17, 2024

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@mgartner mgartner removed the request for review from a team September 17, 2024 19:34
@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Sep 17, 2024
@mgartner mgartner requested a review from rytaft September 17, 2024 19:34
Copy link

blathers-crl bot commented Sep 17, 2024

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner requested a review from DrewKimball October 8, 2024 17:09
@mgartner mgartner force-pushed the backport23.1-129007 branch 2 times, most recently from 7aa89da to bf72a4d Compare October 8, 2024 17:31
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: once the merge conflicts / extra file are resolved.

Reviewed 8 of 28 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @kev-cao, @mgartner, and @rytaft)


pkg/sql/logictest/testdata/logic_test/grant_table line 595 at r2 (raw file):

test           public              NULL                                    schema       public   USAGE           false
test           public              NULL                                    schema       root     ALL             true
=======

Merge conflict here.


pkg/sql/opt/xform/testdata/external/tpcc-later-stats line 638 at r2 (raw file):

<<<<<<< HEAD
 │              │    └── CASE c_credit:37 WHEN 'BC' THEN left((((((c_id:24::STRING || c_d_id:25::STRING) || c_w_id:26::STRING) || '5') || '10') || '3860.61') || c_data:44, 500) ELSE c_data:44::STRING END
=======

Looks like the optimizer tests need to be rewritten as well.


pkg/sql/opt/xform/testdata/external/tpcc-read-committed line 1 at r2 (raw file):

import file=tpcc_schema

This file should be removed.

@mgartner mgartner force-pushed the backport23.1-129007 branch 2 times, most recently from f645fba to 37f4321 Compare October 11, 2024 21:45
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 backport23.1-129007 branch from 37f4321 to 30ad44f Compare October 11, 2024 23:07
@mgartner
Copy link
Collaborator Author

I think this backport would require #108387 too, which has not been backported to 23.1. I'm going to close this backport for now - I don't think there's a strong reason to backport it.

@mgartner mgartner closed this Oct 11, 2024
@mgartner mgartner deleted the backport23.1-129007 branch October 11, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants