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

sql: fix labelled tuple type checking #106553

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Jul 10, 2023

Fixes #106303

Release note (bug fix): A bug has been fixed that caused internal errors
instead of user errors when queries contained labelled tuples with a
different number of elements and labels, e.g., (ROW(1, 2) AS a). This
bug was present since v23.1.0.

@mgartner mgartner requested review from msirek, qiyanghe1998 and a team July 10, 2023 21:28
@mgartner mgartner requested a review from a team as a code owner July 10, 2023 21:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jul 10, 2023
@mgartner mgartner requested a review from DrewKimball July 11, 2023 13:23
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.

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


pkg/sql/sem/tree/type_check.go line 2802 at r1 (raw file):

	// mimics the check in (*Tuple).TypeCheck.
	firstLen := len(first.Exprs)
	if len(first.Labels) > 0 && len(first.Labels) != firstLen {

Should we check other tuples than the first? What happens for a test like:

SELECT CASE WHEN False THEN ROW() ELSE (ROW() AS a) END;

Copy link
Contributor

@msirek msirek 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 all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @qiyanghe1998)

@mgartner mgartner force-pushed the 106303-labelled-tuple-fix branch from 69110a3 to 9c44f03 Compare July 14, 2023 21:31
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! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @qiyanghe1998)


pkg/sql/sem/tree/type_check.go line 2802 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Should we check other tuples than the first? What happens for a test like:

SELECT CASE WHEN False THEN ROW() ELSE (ROW() AS a) END;

Good call. Done.

Fixes cockroachdb#106303

Release note (bug fix): A bug has been fixed that caused internal errors
instead of user errors when queries contained labelled tuples with a
different number of elements and labels, e.g., `(ROW(1, 2) AS a)`. This
bug was present since v23.1.0.
@mgartner mgartner force-pushed the 106303-labelled-tuple-fix branch from 9c44f03 to 19860c2 Compare July 14, 2023 21: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:

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

Copy link
Contributor

@qiyanghe1998 qiyanghe1998 left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@mgartner
Copy link
Collaborator Author

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 17, 2023

Build succeeded:

@craig craig bot merged commit 9187485 into cockroachdb:master Jul 17, 2023
@mgartner mgartner deleted the 106303-labelled-tuple-fix branch July 18, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optbuilder: internal error: tuple contents and labels must be of same length
5 participants