sql: fix typing of CASE statements to match Postgres #108387
Conversation
|
This change is |
There was a problem hiding this comment.
Nice fix!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)
pkg/sql/sem/tree/type_check.go line 2639 at r1 (raw file):
if typ := typedExpr.ResolvedType(); cast.ValidCast(firstValidType, typ, cast.ContextImplicit) { if !cast.ValidCast(typ, firstValidType, cast.ContextImplicit) { firstValidType = typ
Since typeCheckSameTypedExprs is called for expressions other than CASE, and adding this rule could change the ordering of expressions in return value typedExprs, could this cause unexpected behavior for non-CASE expressions?
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/sem/tree/type_check.go line 2639 at r1 (raw file):
Previously, msirek (Mark Sirek) wrote…
Since
typeCheckSameTypedExprsis called for expressions other thanCASE, and adding this rule could change the ordering of expressions in return valuetypedExprs, could this cause unexpected behavior for non-CASE expressions?
Looking at the Postgres code, it seems like this logic is used widely for checking various expressions including COALESCE, IN (...), ARRAY, etc. I think this is what they mean by "UNION, CASE and related constructs". I'm inclined not to backport this change since as you mentioned it could affect a large number of expressions beyond CASE, but I do think it's correct for most if not all of the types that use this function.
Does that sound ok to you?
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/sql/sem/tree/type_check.go line 2639 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Looking at the Postgres code, it seems like this logic is used widely for checking various expressions including COALESCE, IN (...), ARRAY, etc. I think this is what they mean by "UNION, CASE and related constructs". I'm inclined not to backport this change since as you mentioned it could affect a large number of expressions beyond CASE, but I do think it's correct for most if not all of the types that use this function.
Does that sound ok to you?
OK, sounds good. It looks like there's a new test failure in TestLogic_tuple that needs to be figured out too.
There was a problem hiding this comment.
All test failures are fixed. PTAL since I had to make some larger changes to some tests.
Reviewable status:
pkg/sql/sem/tree/type_check.go line 2639 at r1 (raw file):
Previously, msirek (Mark Sirek) wrote…
OK, sounds good. It looks like there's a new test failure in
TestLogic_tuplethat needs to be figured out too.
Thanks, this is fixed now.
There was a problem hiding this comment.
Looks good, and results seem to match Postgres overall.
Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status:
pkg/sql/sem/tree/type_check.go line 2639 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Thanks, this is fixed now.
OK, looks good!
pkg/sql/opt/exec/execbuilder/testdata/scalar line 1292 at r2 (raw file):
query T EXPLAIN (VERBOSE) SELECT * FROM t0 WHERE (CASE WHEN (t0.c0) IN (t0.c0) THEN ARRAY[] ELSE ARRAY[NULL] END) IS NULL
This case also returns ERROR: cannot determine type of empty array on Postgres. Not sure if this is covered by another issue, or if we care.
Fixes cockroachdb#102110 Informs cockroachdb#75103 Informs cockroachdb#108360 Release note (bug fix): Fixed the type resolution logic for CASE statements to more closely match Postgres' logic. In particular, we now adhere to rule 5 listed in https://www.postgresql.org/docs/current/typeconv-union-case.html, which requires that we select the first non-unknown input type as the candidate type, then consider each other non-unknown input type, left to right (CASE treats its ELSE clause (if any) as the "first" input, with the THEN clauses(s) considered after that). If the candidate type can be implicitly converted to the other type, but not vice-versa, select the other type as the new candidate type. Then continue considering the remaining inputs. If, at any stage of this process, a preferred type is selected, stop considering additional inputs (note that CockroachDB does not yet support the concept of a "preferred type").
There was a problem hiding this comment.
Thanks for checking!
bors r+
Reviewable status:
pkg/sql/opt/exec/execbuilder/testdata/scalar line 1292 at r2 (raw file):
Previously, msirek (Mark Sirek) wrote…
This case also returns
ERROR: cannot determine type of empty arrayon Postgres. Not sure if this is covered by another issue, or if we care.
Thanks for checking all of these! I added a TODO to note the difference. Not sure if an issue already exists, but doesn't really seem worth worrying about this case for now.
|
Build succeeded: |
PR cockroachdb#108387 introduced new logic to type-checking that allows nested expressions of a single expression to have different types in some cases when the types can be implicitly casted to a common type. For example, the expression `ARRAY[1::INT, 1::FLOAT8]` would have failed type-checking but is not successfully typed as `[]FLOAT8`. However, cockroachdb#108387 does not add an implicit cast to ensure that each nested expression actually has that type during execution, which causes internal errors in some cases. This commit add the necessary implicit casts. Fixes cockroachdb#109629 There is no release note because the bug is not present in any releases. Release note: None
PR cockroachdb#108387 introduced new logic to type-checking that allows nested expressions of a single expression to have different types in some cases when the types can be implicitly casted to a common type. For example, the expression `ARRAY[1::INT, 1::FLOAT8]` would have failed type-checking but is not successfully typed as `[]FLOAT8`. However, cockroachdb#108387 does not add an implicit cast to ensure that each nested expression actually has that type during execution, which causes internal errors in some cases. This commit add the necessary implicit casts. Fixes cockroachdb#109629 There is no release note because the bug is not present in any releases. Release note: None
PR cockroachdb#108387 introduced new logic to type-checking that allows nested expressions of a single expression to have different types in some cases when the types can be implicitly casted to a common type. For example, the expression `ARRAY[1::INT, 1::FLOAT8]` would have failed type-checking but is not successfully typed as `[]FLOAT8`. However, cockroachdb#108387 does not add an implicit cast to ensure that each nested expression actually has that type during execution, which causes internal errors in some cases. This commit add the necessary implicit casts. Fixes cockroachdb#109629 There is no release note because the bug is not present in any releases. Release note: None
109635: sql: fix nested expression type-checking r=mgartner a=mgartner PR #108387 introduced new logic to type-checking that allows nested expressions of a single expression to have different types in some cases when the types can be implicitly casted to a common type. For example, the expression `ARRAY[1::INT, 1::FLOAT8]` would have failed type-checking but is not successfully typed as `[]FLOAT8`. However, #108387 does not add an implicit cast to ensure that each nested expression actually has that type during execution, which causes internal errors in some cases. This commit add the necessary implicit casts. Fixes #109629 There is no release note because the bug is not present in any releases. Release note: None 109704: roachprod: use more recent ubuntu 20.04 images r=RaduBerinde a=RaduBerinde Epic: none Release note: None Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
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.
Fixes #102110
Informs #75103
Informs #108360
Release note (bug fix): Fixed the type resolution logic for
CASEstatements to more closely match Postgres' logic. In particular, we now adhere to rule 5 listed in https://www.postgresql.org/docs/current/typeconv-union-case.html, which requires that we select the first non-unknown input type as the candidate type, then consider each other non-unknown input type, left to right (CASEtreats itsELSEclause (if any) as the "first" input, with theTHENclauses(s) considered after that). If the candidate type can be implicitly converted to the other type, but not vice-versa, select the other type as the new candidate type. Then continue considering the remaining inputs. If, at any stage of this process, a preferred type is selected, stop considering additional inputs (note that CockroachDB does not yet support the concept of a "preferred type").