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: refactor semantic analysis and fix some bugs #108188

Merged

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Aug 4, 2023

sql/sem/tree: simplify SemaCtx reject flag checks

Release note: None

sql/sem/tree: split derived SemaContext properties from contextual info

Properties derived about expressions during semantic analysis are
communicated to callers via ScalarProperties. Prior to this commit, this
type was also used to provide contextual information while traversing
sub-expressions during semantic analysis. For example, it would indicate
whether the current expression is a descendent of a window function
expression.

These two types of information, derived and contextual, are
fundamentally different. Derived properties bubble up from the bottom of
the tree to the top, while context propagates downward into
sub-expressions. This difference made it difficult to maintaining them
correctly in a single type and difficult to reason about. This commit
introduces the ScalarScene type which is used for providing internal
contextual information during semantic analysis.

Release note: None

sql/sem/tree: do not Restore SemaRejectFlags during semantic analysis

This commit fixes a bug introduced in #105582 that caused
SemaRejectFlags to be restored during semantic analysis, preventing the
analysis from detecting some forms of invalid expressions.

Fixes #108166

There is no release note because the related bug does not exist in any
releases.

Release note: None

sql: do not allow subqueries to be cast to enums in views and UDFs

This commit is a follow-up to #106868 after additional reproductions of
the original bug were found. For now, we disallow any CAST expressions
that contain a subquery in the input and the target type is an ENUM.
I've created #108184 to track this limitation.

Fixes #107654

There is no release note because the release note from #106868 should be
sufficient.

Release note: None

sql/randgen: fix typo in comment

Release note: None

@mgartner mgartner requested review from a team as code owners August 4, 2023 17:27
@mgartner mgartner requested a review from rafiss August 4, 2023 17:27
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner force-pushed the 107654-disallow-type-checking-of-subquery branch from 8f4d630 to 93f2d0b Compare August 4, 2023 17:54
@mgartner mgartner changed the title 107654 disallow type checking of subquery sql: refactor semantic analysis and fix some bugs Aug 4, 2023
@mgartner mgartner force-pushed the 107654-disallow-type-checking-of-subquery branch 2 times, most recently from 41d0327 to 1608fb8 Compare August 7, 2023 12:43
Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

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

I only have comments on naming things.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @rafiss)


pkg/sql/create_view.go line 494 at r4 (raw file):

// does not corrupt the view.
func serializeUserDefinedTypes(
	ctx context.Context, semaCtx *tree.SemaContext, queries string, multiStmt bool, context string,

minor nit: Could we consider renaming context to something like parentName? It's a bit confusing to have context and ctx context.Context.


pkg/sql/sem/tree/type_check.go line 201 at r2 (raw file):

// their descendent expressions can be analyzed with respect to their ancestor
// expression..
type ScalarScene byte

Is scene a software term that I'm not familiar with? I don't think that the word seems to capture what this bit mask is for. Maybe something like ScalarTypeCheckState would be more clear, and then use State wherever we now use Scene?

Properties derived about expressions during semantic analysis are
communicated to callers via ScalarProperties. Prior to this commit, this
type was also used to provide contextual information while traversing
sub-expressions during semantic analysis. For example, it would indicate
whether the current expression is a descendent of a window function
expression.

These two types of information, derived and contextual, are
fundamentally different. Derived properties bubble up from the bottom of
the tree to the top, while context propagates downward into
sub-expressions. This difference made it difficult to maintaining them
correctly in a single type and difficult to reason about. This commit
introduces the ScalarScene type which is used for providing internal
contextual information during semantic analysis.

Release note: None
This commit fixes a bug introduced in cockroachdb#105582 that caused
SemaRejectFlags to be restored during semantic analysis, preventing the
analysis from detecting some forms of invalid expressions.

Fixes cockroachdb#108166

There is no release note because the related bug does not exist in any
releases.

Release note: None
@mgartner mgartner force-pushed the 107654-disallow-type-checking-of-subquery branch from 1608fb8 to 3528ae5 Compare August 7, 2023 17:56
This commit is a follow-up to cockroachdb#106868 after additional reproductions of
the original bug were found. For now, we disallow any CAST expressions
that contain a subquery in the input and the target type is an ENUM.
I've created cockroachdb#108184 to track this limitation.

Fixes cockroachdb#107654

There is no release note because the release note from cockroachdb#106868 should be
sufficient.

Release note: None
@mgartner mgartner force-pushed the 107654-disallow-type-checking-of-subquery branch from 3528ae5 to 25c344c Compare August 7, 2023 17:58
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! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @rafiss, and @rharding6373)


pkg/sql/create_view.go line 494 at r4 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

minor nit: Could we consider renaming context to something like parentName? It's a bit confusing to have context and ctx context.Context.

Yes we can, but we use context for this string elsewhere, like in Properties.Require. I've renamed this case though.


pkg/sql/sem/tree/type_check.go line 201 at r2 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Is scene a software term that I'm not familiar with? I don't think that the word seems to capture what this bit mask is for. Maybe something like ScalarTypeCheckState would be more clear, and then use State wherever we now use Scene?

No, it's just my attempt to avoid the overloaded term context. Thanks for calling this out. How about Ancestors since all the information is about expressions higher up in the expression tree?

Copy link
Collaborator

@rharding6373 rharding6373 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! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @rafiss)


pkg/sql/create_view.go line 494 at r4 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Yes we can, but we use context for this string elsewhere, like in Properties.Require. I've renamed this case though.

Either way then, I'm more neutral on this one.


pkg/sql/sem/tree/type_check.go line 201 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

No, it's just my attempt to avoid the overloaded term context. Thanks for calling this out. How about Ancestors since all the information is about expressions higher up in the expression tree?

Context is a great word! Ancestor seems better in this context, thanks for the change.

@mgartner
Copy link
Collaborator Author

mgartner commented Aug 7, 2023

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 7, 2023

Build succeeded:

@craig craig bot merged commit d6f6de2 into cockroachdb:master Aug 7, 2023
6 of 7 checks passed
@mgartner mgartner deleted the 107654-disallow-type-checking-of-subquery branch August 7, 2023 20:07
@mgartner
Copy link
Collaborator Author

I think we need to backport some of this, as we did #106868.

@mgartner mgartner added backport-22.2.x Flags PRs that need to be backported to 22.2. backport-23.1.x Flags PRs that need to be backported to 23.1 labels Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-22.2.x Flags PRs that need to be backported to 22.2. backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
3 participants