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.

Sign up for GitHub

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

Jump to bottom

sql: CASE with RECORD type and NULL causes internal error with vectorized engine #109105

Closed
rytaft opened this issue Aug 20, 2023 · 10 comments · Fixed by #110325
Closed

sql: CASE with RECORD type and NULL causes internal error with vectorized engine #109105

rytaft opened this issue Aug 20, 2023 · 10 comments · Fixed by #110325
Assignees
@msirek
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. skipped-test T-sql-queries SQL Queries Team

Comments

@rytaft
Copy link
Collaborator

rytaft commented Aug 20, 2023 •
edited by cockroach-jira-scripts
Loading

Describe the problem

Running a query with CASE, where one of the branches is a RECORD type and another branch is NULL, can cause an internal error.

To Reproduce

On master or prior versions, create the following logictest:

statement ok
CREATE TABLE tab (b BOOL);
INSERT INTO tab VALUES (false);

query I
SELECT (CASE WHEN b THEN NULL ELSE ((ROW(1) AS a)) END).a from tab;
----
1

After #108387 merges, replace the last statement with:

SELECT (CASE WHEN b THEN ((ROW(1) AS a)) ELSE NULL END).a from tab

When running with the fakedist config (e.g., ./dev testlogic base --files=testfile --config=fakedist --stress), this produces the following error:

        (XX000) internal error: unable to vectorize execution plan: (CASE WHEN "$0" THEN CAST(NULL AS RECORD) ELSE ((1,) AS a) END).a: could not identify column "a" in record data type
        expr.go:88: in processExpression()
        DETAIL: stack trace:
        github.com/cockroachdb/cockroach/pkg/sql/execinfrapb/pkg/sql/execinfrapb/expr.go:88: processExpression()
        github.com/cockroachdb/cockroach/pkg/sql/execinfrapb/pkg/sql/execinfrapb/expr.go:169: DeserializeExpr()
        github.com/cockroachdb/cockroach/pkg/sql/execinfrapb/pkg/sql/execinfrapb/expr.go:209: Init()
        github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:173: Init()
        github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:795: InitWithEvalCtx()
        github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:764: Init()
        github.com/cockroachdb/cockroach/pkg/sql/rowexec/noop.go:57: newNoopProcessor()
        github.com/cockroachdb/cockroach/pkg/sql/rowexec/processors.go:127: NewProcessor()
        github.com/cockroachdb/cockroach/pkg/sql/colexec/colbuilder/execplan.go:557: func1()
        github.com/cockroachdb/cockroach/pkg/sql/colexec/colbuilder/execplan.go:111: wrapRowSources()
        github.com/cockroachdb/cockroach/pkg/sql/colexec/colbuilder/execplan.go:545: createAndWrapRowSource()
        github.com/cockroachdb/cockroach/pkg/sql/colexec/colbuilder/execplan.go:1856: wrapPostProcessSpec()
        github.com/cockroachdb/cockroach/pkg/sql/colexec/colbuilder/execplan.go:1716: NewColOperator()
        github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:1197: func1()
        github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:92: CatchVectorizedRuntimeError()
        github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:1132: setupFlow()
        github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:260: Setup()
        github.com/cockroachdb/cockroach/pkg/sql/distsql/server.go:429: setupFlow()
        github.com/cockroachdb/cockroach/pkg/sql/distsql/server.go:661: func1()
        github.com/cockroachdb/cockroach/pkg/sql/distsql/server.go:689: SetupFlow()
        github.com/cockroachdb/cockroach/pkg/sql/execinfrapb/bazel-out/darwin_arm64-fastbuild/bin/pkg/sql/execinfrapb/execinfrapb_go_proto_/github.com/cockroachdb/cockroach/pkg/sql/execinfrapb/api.pb.go:602: func1()
        github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor/grpc_interceptor.go:97: func1()
        google.golang.org/grpc/external/org_golang_google_grpc/server.go:1163: func1()
        github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:169: func3()
        google.golang.org/grpc/external/org_golang_google_grpc/server.go:1163: func1()
        github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/auth.go:105: unaryInterceptor()
        google.golang.org/grpc/external/org_golang_google_grpc/server.go:1163: func1()
        github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:136: 1()
        github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:336: RunTaskWithErr()
        github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:134: func1()
        google.golang.org/grpc/external/org_golang_google_grpc/server.go:1154: func1()
        github.com/cockroachdb/cockroach/pkg/sql/execinfrapb/bazel-out/darwin_arm64-fastbuild/bin/pkg/sql/execinfrapb/execinfrapb_go_proto_/github.com/cockroachdb/cockroach/pkg/sql/execinfrapb/api.pb.go:604: _DistSQL_SetupFlow_Handler()

Expected behavior
This should not cause an internal error.

Jira issue: CRDB-30782
@rytaft rytaft added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 20, 2023
@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Aug 20, 2023
@mgartner mgartner moved this from Triage to 23.2 Release in SQL Queries Aug 22, 2023
@mgartner
Copy link
Collaborator

TODO: determine if we should fix this for 23.2 once #108387 is merged.

@rafiss
Copy link
Collaborator

rafiss commented Aug 24, 2023

From what I understand the SELECT (CASE WHEN b THEN ((ROW(1) AS a)) ELSE NULL END).a from t78159 test that was commented out is because there was an internal error happening in #78159 - is it likely for that to happen again?

@msirek msirek mentioned this issue Aug 26, 2023
Closed
@msirek
Copy link
Contributor

msirek commented Aug 26, 2023

This issue should also handle the internal error found in #109486.

@msirek msirek self-assigned this Aug 27, 2023
@msirek msirek moved this from 23.2 Release to Active in SQL Queries Aug 27, 2023
@mgartner
Copy link
Collaborator

I think this might be similar or that same issue as #109629.

@msirek
Copy link
Contributor

msirek commented Aug 28, 2023

I think this might be similar or that same issue as #109629.

@mgartner Thanks. It looks similar. My current fix is adding implicit casts, but it's not currently handling the test case in #109629. I'll take a look and see what's going on.

@mgartner
Copy link
Collaborator

Hmm. I have a fix in #109635 that is working for #109629.

@msirek
Copy link
Contributor

msirek commented Aug 28, 2023

Hmm. I have a fix in #109635 that is working for #109629.

OK, looks like you beat me to it. OK, I'll try some of my other test cases against your fix.

@rafiss rafiss mentioned this issue Aug 29, 2023
Closed
@mgartner
Copy link
Collaborator

Looks like #109635 does not fix this issue.

@msirek
Copy link
Contributor

msirek commented Sep 8, 2023

Tested against final version of #109635. Closing as a dup.

@msirek msirek closed this as completed Sep 8, 2023
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Sep 8, 2023
@rafiss rafiss added skipped-test T-sql-queries SQL Queries Team labels Sep 8, 2023
@rafiss
Copy link
Collaborator

rafiss commented Sep 8, 2023

reopening since a skipped test still references this issue

cockroach/pkg/sql/logictest/testdata/logic_test/tuple

Lines 1204 to 1208 in ef2991c

# TODO(rytaft): Uncomment this case once #109105 is fixed.
# query B
# SELECT (CASE WHEN b THEN ((ROW(1) AS a)) ELSE NULL END).a from t78159
# ----
# NULL

@rafiss rafiss reopened this Sep 8, 2023
@github-project-automation github-project-automation bot moved this from Done to Triage in SQL Queries Sep 8, 2023
msirek pushed a commit to msirek/cockroach that referenced this issue Sep 8, 2023
logictest: Uncomment t78159 test case
2a617ea 
This uncomments a unit test from cockroachdb#78159 which was fixed by cockroachdb#109635.

Informs: cockroachdb#109105

Release note: None
@msirek msirek mentioned this issue Sep 8, 2023
Closed
@msirek msirek mentioned this issue Sep 10, 2023
Closed
msirek pushed a commit to msirek/cockroach that referenced this issue Sep 10, 2023
tree: make typeCheckSameTypedExprs order invariant for tuples
9e526be 
Type checking of CASE expressions (and other expressions handled by
typeCheckSameTypedExprs) does not type check tuples properly because
typeCheckSameTypedTupleExprs checking is only done when the tuple is the
first expression in `exprs`.

This is fixed by finding the first tuple in `exprs`, searching the slice
starting at index 0, and using that to drive typeCheckSameTypedExprs.

Fixes: cockroachdb#109105

Release note: None
msirek pushed a commit to msirek/cockroach that referenced this issue Sep 10, 2023
tree: apply casts in typeCheckSameTypedExprs for non-equivalent types
adf7103 
Function typeCheckSameTypedExprs is updated by cockroachdb#108387 and cockroachdb#109635 to
apply implicit CASTs to the different expressions input to an array,
tuple, case expression or other similar expression, to coerce them all
to a common data type, instead of erroring out. This is not done,
however, if the data types of these expressions are not equivalent with
each other, causing some cases to error out.

The fix is to match Postgres behavior and apply the casts even for
non-equivalent types.

Informs: cockroachdb#109105

Release note (sql change): This patch modifies type checking of arrays,
tuples, and case statements to allow implicit casting of scalar
expressions referenced in these constructs to a common data type, even
for types in different type families, as long at the implicit cast is
legal.
@msirek msirek mentioned this issue Sep 11, 2023
Merged
@yuzefovich yuzefovich moved this from Triage to Active in SQL Queries Sep 12, 2023
craig bot pushed a commit that referenced this issue Sep 28, 2023
Merge #110325
2111b61 
110325: tree: make typeCheckSameTypedExprs order invariant for tuples r=msirek a=msirek

Type checking of CASE expressions (and other expressions handled by typeCheckSameTypedExprs) does not type check tuples properly because typeCheckSameTypedTupleExprs checking is only done when the tuple is the first expression in `exprs`.

This is fixed by finding the first tuple in `exprs`, searching the slice starting at index 0, and using that to drive typeCheckSameTypedExprs.

Fixes: #109105

Release note: None

Co-authored-by: Mark Sirek <[email protected]>
@craig craig bot closed this as completed in 593c830 Sep 28, 2023
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Sep 28, 2023
THardy98 pushed a commit to THardy98/cockroach that referenced this issue Oct 6, 2023
tree: make typeCheckSameTypedExprs order invariant for tuples
8d19e0e 
Type checking of CASE expressions (and other expressions handled by
typeCheckSameTypedExprs) does not type check tuples properly because
typeCheckSameTypedTupleExprs checking is only done when the tuple is the
first expression in `exprs`.

This is fixed by finding the first tuple in `exprs`, searching the slice
starting at index 0, and using that to drive typeCheckSameTypedExprs.

Fixes: cockroachdb#109105

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@msirek msirek

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. skipped-test T-sql-queries SQL Queries Team
Projects
SQL Queries
Archived in project
Milestone
No milestone
4 participants
@mgartner @rafiss @rytaft @msirek