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: wrap virtual computed column projections in a cast #129008

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

mgartner
Copy link
Collaborator

Backport 3/3 commits from #105736.

/cc @cockroachdb/release


sql/logictest: remove assignment cast TODO

This commit removes a TODO that was partially addressed by #82022.

Informs #73743

Release note: None

sql/types: fix T.Identical

This commit fixes a bug in types.T.Identical which caused it to return
false for collated string types with a different string-representation
of locales that represents the same locale logically. For example,
collated string types with locales en_US and en_us would not be
identical, even though these are both valid representations of the same
locale.

There is no release note because this has not caused any known bugs.

Release note: None

opt: wrap virtual computed column projections in a cast

optbuilder wraps an assignment cast around a virtual computed column
expression when writing to the table if the expression does not have the
same type as the virtual column. This matches the behavior of all writes
to columns of expressions that don't match the target column type.

However, the same cast was not applied to the projection of the virtual
column expression when reading from the table. This cast is necessary in
order to produce the correct value of the column - the projection must
produce the same value that would have been written to the table if the
column was a stored computed column. This commit corrects optbuilder so
that the cast is correctly added to the projection.

Note that this commit adds a standard cast, not an assignment cast, as a
temporary workaround until #81698 is addressed. This is because an
assignment cast can fail in cases when a regular cast will not. Because
we don't validate that all existing rows can successfully produce a new
virtual column expression during a backfill, adding an assignment cast
to the projection of the virtual column could cause future reads to
error. Once #81698 is addressed, we can change these casts to assignment
casts so that they directly match the values that would be written to
the same column if it were stored.

Fixes #91817
Informs #81698

Release note (bug fix): A bug has been fixed that could produce incorrect
values for virtual computed columns in rare cases. The bug only
presented when the virtual column expression's type did not match the
type of the virtual column.


Release justification: Minor bug fix that will silence test failures.

This commit removes a TODO that was partially addressed by cockroachdb#82022.

Informs cockroachdb#73743

Release note: None
This commit fixes a bug in `types.T.Identical` which caused it to return
false for collated string types with a different string-representation
of locales that represents the same locale logically. For example,
collated string types with locales `en_US` and `en_us` would not be
identical, even though these are both valid representations of the same
locale.

There is no release note because this has not caused any known bugs.

Release note: None
optbuilder wraps an assignment cast around a virtual computed column
expression when writing to the table if the expression does not have the
same type as the virtual column. This matches the behavior of all writes
to columns of expressions that don't match the target column type.

However, the same cast was not applied to the projection of the virtual
column expression when reading from the table. This cast is necessary in
order to produce the correct value of the column - the projection must
produce the same value that would have been written to the table if the
column was a stored computed column. This commit corrects optbuilder so
that the cast is correctly added to the projection.

Note that this commit adds a standard cast, not an assignment cast, as a
temporary workaround until cockroachdb#81698 is addressed. This is because an
assignment cast can fail in cases when a regular cast will not. Because
we don't validate that all existing rows can successfully produce a new
virtual column expression during a backfill, adding an assignment cast
to the projection of the virtual column could cause future reads to
error. Once cockroachdb#81698 is addressed, we can change these casts to assignment
casts so that they directly match the values that would be written to
the same column if it were stored.

Fixes cockroachdb#91817
Informs cockroachdb#81698

Release note (bug fix): A bug has been fixed that could produce incorrect
values for virtual computed columns in rare cases. The bug only
presented when the virtual column expression's type did not match the
type of the virtual column.
@mgartner mgartner requested a review from a team August 14, 2024 20:35
@mgartner mgartner requested review from a team as code owners August 14, 2024 20:35
@mgartner mgartner requested review from michae2 and removed request for a team August 14, 2024 20:35
Copy link

blathers-crl bot commented Aug 14, 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.

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Aug 14, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@michae2 michae2 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 1 files at r1, 2 of 2 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

@mgartner mgartner merged commit d08df82 into cockroachdb:release-23.1 Aug 15, 2024
6 checks passed
@mgartner mgartner deleted the backport23.1-105736 branch August 15, 2024 13:33
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