sql: do not type-check subqueries outside of optbuilder #106868

mgartner · 2023-07-14T20:41:34Z

This commit fixes a bug that was caused when attempting to re-type-check
a view's query that contains a subquery. This type-checking occurred
outside of optbuilder. However, the logic for type-checking subqueries
is only implemented within optbuilder, so it failed.

Fixes #105259

Release note (bug fix): A bug has been fixed that caused internal errors
when using user-defined types in views and user-defined functions that
have subqueries. This bug was present when using views since version
v21.2. It was present when using user-defined functions since v23.1.

cockroach-teamcity · 2023-07-14T20:41:44Z

This change is

rafiss

nice! glad we can keep it simple

rafiss · 2023-07-14T21:59:22Z

pkg/sql/logictest/testdata/logic_test/udf

+
+# Renaming the enum value corrupts the UDF. This is expected behavior.
+statement error pgcode 22P02 invalid input value for enum e105259: "foo"
+SELECT f()


nit: add a subtest end. (and also in the next file)

rharding6373

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @rafiss)

pkg/sql/logictest/testdata/logic_test/udf line 3646 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add a subtest end. (and also in the next file)

Educational question (since I only just started noticing subtest end in test files a couple days ago, so it doesn't seem to be used consistently): What is the benefit of using subtest end? It seems like subtests run correctly without it?

rafiss

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @rharding6373)

pkg/sql/logictest/testdata/logic_test/udf line 3646 at r1 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Educational question (since I only just started noticing subtest end in test files a couple days ago, so it doesn't seem to be used consistently): What is the benefit of using subtest end? It seems like subtests run correctly without it?

they are not needed for correctness - the benefit is just that a future change that continues adding tests at the end of the file will not incorrectly be grouped with the subtest that was never ended. it can make things like viewing failures in teamcity harder to understand if that happens.

This commit fixes a bug that was caused when attempting to re-type-check a view's query that contains a subquery. This type-checking occurred outside of optbuilder. However, the logic for type-checking subqueries is only implemented within optbuilder, so it failed. Fixes cockroachdb#105259 Release note (bug fix): A bug has been fixed that caused internal errors when using user-defined types in views and user-defined functions that have subqueries. This bug was present when using views since version v21.2. It was present when using user-defined functions since v23.1.

mgartner

nice! glad we can keep it simple

We can keep it simple for now, but I think in the long-term we should avoid this re-type checking entirely. We should be able to annotate the original AST and pass it along to the execution of these DDL statements so that the AST can be serialized correctly without having to traverse it again. And ideally, checks for things like cross-db references will happen in the optimizer and error-out early.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan)

pkg/sql/logictest/testdata/logic_test/udf line 3646 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

they are not needed for correctness - the benefit is just that a future change that continues adding tests at the end of the file will not incorrectly be grouped with the subtest that was never ended. it can make things like viewing failures in teamcity harder to understand if that happens.

Done. I sprinkled a view more in too. I'll be more diligent about requiring these in the future.

chengxiong-ruan

heh, easier than I thought!

mgartner · 2023-07-17T16:25:43Z

TFTRs!

bors r+

craig · 2023-07-17T17:32:45Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-07-17T17:32:51Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from bd38886 to blathers/backport-release-22.2-106868: 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.

error creating merge commit from bd38886 to blathers/backport-release-23.1-106868: 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 23.1.x failed. See errors above.

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

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

108188: sql: refactor semantic analysis and fix some bugs r=mgartner a=mgartner #### 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 Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

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 requested review from chengxiong-ruan, rharding6373 and a team July 14, 2023 20:41

mgartner requested a review from a team as a code owner July 14, 2023 20:41

mgartner force-pushed the 105259-simple-fix branch from c1487c8 to a620361 Compare July 14, 2023 20:42

mgartner changed the title ~~sql: do not type-check subqueries in views outside of optbuilder~~ sql: do not type-check subqueries outside of optbuilder Jul 14, 2023

mgartner added backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 labels Jul 14, 2023

rafiss approved these changes Jul 14, 2023

View reviewed changes

rharding6373 approved these changes Jul 14, 2023

View reviewed changes

rafiss reviewed Jul 14, 2023

View reviewed changes

mgartner force-pushed the 105259-simple-fix branch from a620361 to bd38886 Compare July 17, 2023 14:33

mgartner commented Jul 17, 2023

View reviewed changes

chengxiong-ruan approved these changes Jul 17, 2023

View reviewed changes

craig bot merged commit 1778bf7 into cockroachdb:master Jul 17, 2023
7 checks passed

mgartner deleted the 105259-simple-fix branch July 17, 2023 19:30

This was referenced Jul 17, 2023

release-23.1: sql: do not type-check subqueries outside of optbuilder #106955

Merged

release-22.2: sql: do not type-check subqueries outside of optbuilder #106956

Merged

mgartner mentioned this pull request Jul 26, 2023

Assertion failure during unoptimized-query-oracle/disable-rules=all/rand-tables test #107654

Closed

mgartner mentioned this pull request Aug 4, 2023

sql: allow casting subqueries to ENUMs in views and UDFs #108184

Open

mgartner mentioned this pull request Aug 4, 2023

sql: refactor semantic analysis and fix some bugs #108188

Merged

mgartner mentioned this pull request Aug 22, 2023

release-23.1: sql: refactor semantic analysis and fix some bugs #109237

Merged

This was referenced Aug 22, 2023

release-22.2: sql: refactor semantic analysis and fix some bugs #109241

Merged

pkg/ccl/testccl/sqlccl/sqlccl_test: TestShowCreateRedactableValues failed #108894

Closed

mgartner mentioned this pull request Aug 23, 2023

release-23.1.9-rc: sql: refactor semantic analysis and fix some bugs #109331

Merged

mgartner mentioned this pull request Oct 25, 2023

pkg/ccl/testccl/sqlccl/sqlccl_test: TestShowCreateRedactableValues failed #112750

Closed

DrewKimball mentioned this pull request Apr 10, 2024

sql: don't type-check subqueries in routine argument defaults #122085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: do not type-check subqueries outside of optbuilder #106868

sql: do not type-check subqueries outside of optbuilder #106868

mgartner commented Jul 14, 2023

cockroach-teamcity commented Jul 14, 2023

rafiss left a comment

rafiss Jul 14, 2023

rharding6373 left a comment

rafiss left a comment

mgartner left a comment

chengxiong-ruan left a comment

mgartner commented Jul 17, 2023

craig bot commented Jul 17, 2023

blathers-crl bot commented Jul 17, 2023

sql: do not type-check subqueries outside of optbuilder #106868

sql: do not type-check subqueries outside of optbuilder #106868

Conversation

mgartner commented Jul 14, 2023

cockroach-teamcity commented Jul 14, 2023

rafiss left a comment

Choose a reason for hiding this comment

rafiss Jul 14, 2023

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

chengxiong-ruan left a comment

Choose a reason for hiding this comment

mgartner commented Jul 17, 2023

craig bot commented Jul 17, 2023

blathers-crl bot commented Jul 17, 2023