exec: Fix NULL expression handling in CASE and AND operators #40657
Conversation
|
This change is |
rohany
force-pushed
the
const-nulls
branch
3 times, most recently
from
9283991
to
16c5901
Compare
There was a problem hiding this comment.
Need to update the commit message to Release justificatoin: blah to satisfy the linter now.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @rohany, and @yuzefovich)
pkg/sql/distsqlrun/column_exec_setup.go, line 962 at r1 (raw file):
// plan constNullOperators in the case that we know the "type" of the null. It is currently // unsafe to plan a constNullOperator when we don't know the type of the null. func planTypedMaybeNullProjectionOperators(
This is fine but isn't it unfortunate that everyone going forward will have to remember which to use? Can we make the other variant return an error if its type is unknown? That should always be an error right?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/column_exec_setup.go, line 962 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
This is fine but isn't it unfortunate that everyone going forward will have to remember which to use? Can we make the other variant return an error if its type is unknown? That should always be an error right?
The other variant does now in this PR. I guess its less of remember which one to use and consciously pick this one when you know you can handle a null value.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/column_exec_setup.go, line 962 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
The other variant does now in this PR. I guess its less of remember which one to use and consciously pick this one when you know you can handle a null value.
- the other variant does throw an error
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/column_exec_setup.go, line 962 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
- the other variant does throw an error
Ahh I didn't notice, but it's obvious now that you point it out. SGTM.
rohany
force-pushed
the
const-nulls
branch
3 times, most recently
from
87ce20d
to
f4a67b6
Compare
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Reviewable status:
This change fixes the CASE and AND operators when nulls were used as expressions, which caused different errors than nulls existing as data within the batches. Additionally, this change increases the safety of the vectorized engine by disallowing the engine to plan constNullOperators without knowing the "type" of the null to avoid having the same panic arise. To get around this, when the "type" of the null is known, a constNullOperator with a known type can be planned. This is a temporary fix for the release. To be properly fixed, datum nulls need to be aware of their type post typechecking, instead of being "unknown". Release justification: This PR fixes known panics within the vectorized engine. Fixes cockroachdb#40526. Release note: None
|
bors r+ |
Build failed |
|
I have a release justification though... bors r+ |
40657: exec: Fix NULL expression handling in CASE and AND operators r=rohany a=rohany This change fixes the CASE and AND operators when nulls were used as expressions, which caused different errors than nulls existing as data within the batches. Additionally, this change increases the safety of the vectorized engine by disallowing the engine to plan constNullOperators without knowing the "type" of the null to avoid having the same panic arise. To get around this, when the "type" of the null is known, a constNullOperator with a known type can be planned. This is a temporary fix for the release. To be properly fixed, datum nulls need to be aware of their type post typechecking, instead of being "unknown". Release justification: This PR fixes known panics within the vectorized engine. Fixes #40526. Release note: None Co-authored-by: Rohan Yadav <rohany@cockroachlabs.com>
Build succeeded |
This change fixes the CASE and AND operators when nulls were used as
expressions, which caused different errors than nulls existing as data
within the batches. Additionally, this change increases the safety of
the vectorized engine by disallowing the engine to plan
constNullOperators without knowing the "type" of the null to avoid
having the same panic arise. To get around this, when the "type" of the
null is known, a constNullOperator with a known type can be planned.
This is a temporary fix for the release. To be properly fixed, datum
nulls need to be aware of their type post typechecking, instead of being
"unknown".
Release justification: This PR fixes known panics within the vectorized engine.
Fixes #40526.
Release note: None