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

parser: fix GetTypeFromValidSQLSyntax for collated strings #96695

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Feb 7, 2023

Previously, GetTypeFromValidSQLSyntax would result in an error when attempting to parse a collated string. This is due to an expression like 1::STRING COLLATE en being parsed as CollateExpr(CastExpr) whereas the function expected just CastExpr. This is now fixed by having a special case for the collated strings.

Additionally, this commit adds a couple of testing improvements around the collated strings:

  • a representative collated string type is now included into randgen.SeedTypes (this actually exposed a minor bug in recently introduced tree.DatumNext method)
  • a new test is introduced that asserts that for all (with a few exceptions) type families at least one representative type is included into randgen.SeedTypes.

Fixes: #77353.

Release note (bug fix): Previously, ALTER TABLE ... INJECT STATISTICS command would fail if a column with COLLATED STRING type had histograms to be injected, and this is now fixed. The bug has been present since at least 21.2.

@yuzefovich yuzefovich requested review from rafiss and a team February 7, 2023 00:15
@yuzefovich yuzefovich requested a review from a team as a code owner February 7, 2023 00:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich
Copy link
Member Author

There are some interesting failures, will need to spend more time. Hold off on reviewing for now.

@yuzefovich yuzefovich marked this pull request as draft February 7, 2023 18:34
@yuzefovich yuzefovich force-pushed the collate branch 2 times, most recently from c6f5ef5 to 59c8bb3 Compare February 14, 2023 16:12
@yuzefovich yuzefovich marked this pull request as ready for review February 14, 2023 16:49
@yuzefovich yuzefovich requested a review from a team as a code owner February 14, 2023 16:49
@yuzefovich yuzefovich requested review from herkolategan and renatolabs and removed request for a team February 14, 2023 16:49
@yuzefovich
Copy link
Member Author

RFAL. @rafiss let me know if you think someone else should be reviewing this.

@yuzefovich
Copy link
Member Author

Friendly ping @rafiss

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

sorry for missing this! this lgtm! i had some optional comments on the release note and a test.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)


-- commits line 18 at r1:
seems like #77353 was a user-facing bug report. so i think we could have a release note like "fixed an error that could happen when capturing statement diagnostics for a query that references a COLLATE expression"


pkg/sql/parser/parse_test.go line 761 at r1 (raw file):

		// TODO(yuzefovich): ideally, we'd assert that the returned type is
		// equal to the original one; however, there are some subtle differences
		// at the moment (like the width might only be set on the returned

can't we still assert that the type OIDs are the same though?

Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


-- commits line 18 at r1:

Previously, rafiss (Rafi Shamim) wrote…

seems like #77353 was a user-facing bug report. so i think we could have a release note like "fixed an error that could happen when capturing statement diagnostics for a query that references a COLLATE expression"

Good point, done.


pkg/sql/parser/parse_test.go line 761 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can't we still assert that the type OIDs are the same though?

Done.

@craig
Copy link
Contributor

craig bot commented Feb 22, 2023

Build failed:

@yuzefovich
Copy link
Member Author

Seems that I need to adjust one of recently merged tests.

bors r-

Previously, `GetTypeFromValidSQLSyntax` would result in an error when
attempting to parse a collated string. This is due to an expression like
`1::STRING COLLATE en` being parsed as `CollateExpr(CastExpr)` whereas
the function expected just `CastExpr`. This is now fixed by having
a special case for the collated strings.

Additionally, this commit adds a couple of testing improvements around
the collated strings:
- a representative collated string type is now included into
`randgen.SeedTypes` (this actually exposed a minor bug in recently
introduced `tree.DatumNext` method)
- a new test is introduced that asserts that for all (with a few
exceptions) type families at least one representative type is included
into `randgen.SeedTypes`.

Release note (bug fix): Previously, `ALTER TABLE ... INJECT STATISTICS`
command would fail if a column with COLLATED STRING type had histograms
to be injected, and this is now fixed. The bug has been present since at
least 21.2.
@yuzefovich
Copy link
Member Author

It was actually a bug introduced in #97152. The impact of that bug is minor, so I just included the fix into this PR.

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 22, 2023

Build succeeded:

@craig craig bot merged commit 58297f3 into cockroachdb:master Feb 22, 2023
@blathers-crl
Copy link

blathers-crl bot commented Feb 22, 2023

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 1902d3d to blathers/backport-release-22.1-96695: 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 22.1.x failed. See errors above.


error creating merge commit from 1902d3d to blathers/backport-release-22.2-96695: 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 22.2.x failed. See errors above.


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

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

Successfully merging this pull request may close these issues.

Use of COLLATE in a statement bundle stats file causes error during injection
3 participants