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

collatedstring: support default, C, and POSIX in expressions #96828

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Feb 8, 2023

fixes #50734
fixes #95667
informs #57255


collatedstring: create new package

Move the small amount of code from tree/collatedstring.go


collatedstring: support C and POSIX in expressions

Release note (sql change): Expressions of the form COLLATE "default",
COLLATE "C", and COLLATE "POSIX" are now supported. Since the
default collation cannot be changed currently, these expressions are all
equivalent. The expressions are evaluated by treating the input as a
normal string, and ignoring the collation.

This means that comparisons between strings and collated strings that
use "default", "C", or "POSIX" are now supported.

Creating a column with the "C" or "POSIX" collations is still not
supported.

Move the small amount of code from tree/collatedstring.go

Release note: None
@rafiss rafiss requested review from otan and a team February 8, 2023 22:24
@rafiss rafiss requested a review from a team as a code owner February 8, 2023 22:24
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss changed the title collatedstring: support C and POSIX in expressions collatedstring: support default, C, and POSIX in expressions Feb 8, 2023
@rafiss rafiss force-pushed the collate-default-alias branch from 095263e to 4bdb998 Compare February 8, 2023 22:34
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

two broader questions:

// modified.
const PosixCollationTag = "POSIX"

var supportedTagNames []string
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use language.Tag here?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, maybe we can't because language.Tag has metadata in it. oh well!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah i had tried to get it to work with Tag, but it can't. there's no way to even make a "custom" Tag.

@rafiss rafiss force-pushed the collate-default-alias branch from 4bdb998 to b8a80ca Compare February 8, 2023 23:32
@blathers-crl blathers-crl bot requested a review from otan February 8, 2023 23:32
@rafiss
Copy link
Collaborator Author

rafiss commented Feb 8, 2023

does pg_catalog.pg_collation need an update?

that's done as part of this

should we add a linter that prevents usage of collate.Supported()?

added

Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

ah the logic tests only mention information_schema.collations. i can see the code tho - feel free to add the test if you feel it's worth.

Release note (sql change): Expressions of the form `COLLATE "default"`,
`COLLATE "C"`, and `COLLATE "POSIX"` are now supported. Since the
default collation cannot be changed currently, these expressions are all
equivalent. The expressions are evaluated by treating the input as a
normal string, and ignoring the collation.

This means that comparisons between strings and collated strings that
use "default", "C", or "POSIX" are now supported.

Creating a column with the "C" or "POSIX" collations is still not
supported.
@rafiss rafiss force-pushed the collate-default-alias branch from b8a80ca to a7e9c4a Compare February 9, 2023 06:27
@rafiss
Copy link
Collaborator Author

rafiss commented Feb 9, 2023

tftr!

bors r=otan

@craig
Copy link
Contributor

craig bot commented Feb 9, 2023

Build failed (retrying...):

@craig craig bot merged commit 732c3a7 into cockroachdb:master Feb 9, 2023
@craig
Copy link
Contributor

craig bot commented Feb 9, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants