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

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

Closed
renatolabs opened this issue Jul 26, 2023 · 4 comments · Fixed by #108188
Closed
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). O-rsg Random Syntax Generator T-sql-queries SQL Queries Team
Projects

Comments

@renatolabs
Copy link
Collaborator

renatolabs commented Jul 26, 2023

I was running a sample of roachtests on my PR (#107548) and noticed that the unoptimized-query-oracle/disable-rules=all/rand-tables test failed due to an assertion failure in the cockroach node:

E230726 03:19:06.241395 2499 sql/sqltelemetry/report.go:57 ⋮ [T1,n1,client=35.230.180.232:57860,hostnossl,user=root] 6246 +building declarative schema change targets for CREATE FUNCTION: ReturnType called on TypedExpr with empty typeAnnotation. Was the underlying Expr type-checked before asserting a type of TypedExpr?

I don't think this is related to the diff on my PR (mostly changes in the roachtest monitor), so I'm opening a separate issue for this. Artifacts can be found at:

https://teamcity.cockroachdb.com/viewLog.html?buildId=11055808&buildTypeId=Cockroach_Nightlies_RoachtestNightlyGceBazel&tab=buildResultsDiv

Jira issue: CRDB-30148

@renatolabs renatolabs added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). T-sql-queries SQL Queries Team labels Jul 26, 2023
@blathers-crl blathers-crl bot added this to Triage in SQL Queries Jul 26, 2023
@mgartner
Copy link
Collaborator

mgartner commented Jul 26, 2023

Looks like another case of #105259. AFAICT the PR had the fix #106868, so this seems to be a different case. I was able to put together a reproduction from the stack trace in the logs:

CREATE TYPE e AS ENUM ('foo');

CREATE VIEW v AS SELECT (
  CASE WHEN true THEN (SELECT 'foo') ELSE NULL END
)::e;

So we need a more robust solution than #106868.

@mgartner mgartner assigned mgartner and unassigned mgartner Jul 26, 2023
@mgartner
Copy link
Collaborator

I spent an afternoon trying to fix this, and unfortunately it's fairly tricky. We really shouldn't be re-type-checking the AST outside of the optimizer - that's the real offender here. Unfortunately, in order to stop doing that, we'll need to do something like annotating the AST during type checking with information that can be used to serialize enum values and types during formatting. I went down this path and think there's potential, but I'd be hard-pressed to back port it.

Next I tried a more backportable approach that would only convert enums and types in the form 'foo'::enum. But this is insufficient for expressions like ARRAY['foo']::enum[]. And I'm sure there's many other expressions that we'd miss if we tried to handle specific cases.

So I think annotating the AST is our best bet, but it's a significant amount of work and not backportable.

@mgartner
Copy link
Collaborator

The draft PR of my annotations is here: #107678

It's not really sufficient because it wouldn't correctly cover the case mentioned above either, ARRAY['foo']::enum[]. So consider it a prototype at best...

@mgartner
Copy link
Collaborator

mgartner commented Aug 2, 2023

I don't see a simple way to fix this, so for now I think we should detect usage of UDTs within subqueries and return an "unimplemented" error.

mgartner added a commit to mgartner/cockroach that referenced this issue Aug 7, 2023
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 added a commit to mgartner/cockroach that referenced this issue Aug 7, 2023
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
craig bot pushed a commit that referenced this issue Aug 7, 2023
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>
@craig craig bot closed this as completed in 9fd51a0 Aug 7, 2023
mgartner added a commit to mgartner/cockroach that referenced this issue Aug 22, 2023
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 added a commit to mgartner/cockroach that referenced this issue Aug 22, 2023
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 added a commit to mgartner/cockroach that referenced this issue Aug 23, 2023
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 added a commit to mgartner/cockroach that referenced this issue Aug 23, 2023
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 added a commit to mgartner/cockroach that referenced this issue Aug 23, 2023
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
@michae2 michae2 added the O-rsg Random Syntax Generator label Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). O-rsg Random Syntax Generator T-sql-queries SQL Queries Team
Projects
Archived in project
SQL Queries
Triage (Old)
Development

Successfully merging a pull request may close this issue.

3 participants