opt: check that generator functions are not used in CASE or COALESCE #105582
Conversation
|
This change is |
|
Hold off on this one for a bit; I think I added to many restrictions for what expressions can exist inside a COALESCE or CASE. |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
-- commits line 12 at r1:
Can you mention IFexpressions too? We build those as CaseExprs in optbuilder, so I don't think you'll need to change any code - though might be good to add some IF test cases like SELECT IF(false, generate_series(1, 3), 1).
There was a problem hiding this comment.
This is RFAL.
Reviewable status:
Previously, mgartner (Marcus Gartner) wrote…
Can you mention
IFexpressions too? We build those as CaseExprs in optbuilder, so I don't think you'll need to change any code - though might be good to add someIFtest cases likeSELECT IF(false, generate_series(1, 3), 1).
Done.
DrewKimball
requested review from
srosenberg and
renatolabs
and removed request for
a team
DrewKimball
force-pushed
the
case
branch
2 times, most recently
from
4a59fa7
to
75d32a0
Compare
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 17 of 17 files at r2, all commit messages.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/srfs line 1383 at r2 (raw file):
# IFNULL does not allow generator functions. statement error pq: set-returning functions are not allowed in COALESCE
Is COALESCE a typo, or is this message due to a normalization of IFNULL to a COALESCE?
pkg/sql/opt/optbuilder/testdata/aggregate line 3636 at r2 (raw file):
SELECT array_agg(generate_series(1, 2)) ---- error (0A000): array_agg(): set-returning functions are not allowed in aggregate
Is this change intentional? I don't see anything in the release note mentioning aggregate functions.
This patch adds checks during type-checking to ensure that generator functions are not used in the arguments of `CASE`, `IF`, `COALESCE`, or `IFNULL` expressions. This mirrors postgres behavior. This patch also corrects the error message that is returned in these cases to say "set-returning" instead of "generator". Fixes cockroachdb#97119 Fixes cockroachdb#94890 Release note (bug fix): CASE, IF, COALESCE, and IFNULL expressions now return an error when passed a generator function as an argument. This mirrors postgres behavior.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/srfs line 1383 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Is
COALESCEa typo, or is this message due to a normalization of IFNULL to a COALESCE?
This is because we parse IFNULL directly as a COALESCE, so there's never even a tree expression for IFNULL. Expanded the comment.
pkg/sql/opt/optbuilder/testdata/aggregate line 3636 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Is this change intentional? I don't see anything in the release note mentioning aggregate functions.
This change is correct, we weren't catching this case previously because replaceSRF was called before we checked if generator functions were allowed. Now srf.TypeCheck also checks if the context allows generators, so we throw the error.
There was a problem hiding this comment.
Nicely done!
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/srfs line 1383 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
This is because we parse
IFNULLdirectly as aCOALESCE, so there's never even atreeexpression forIFNULL. Expanded the comment.
👍
pkg/sql/opt/optbuilder/testdata/aggregate line 3636 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
This change is correct, we weren't catching this case previously because
replaceSRFwas called before we checked if generator functions were allowed. Nowsrf.TypeCheckalso checks if the context allows generators, so we throw the error.
👍
|
TFTR! bors r+ |
|
Build succeeded: |
A change in cockroachdb#105582 caused the regions/replicas query to return incorrect, unexpected results causing the endpoint to never resolve properly, which in turn, caused an infinite loading state. This change fixes the regions/replicas query for the database details API. Release note (ui change): fix a broken query for the database details page that was causing an infinite loading state.
107893: cluster-ui: fix replicas and regions query for the database details API r=THardy98 a=THardy98 Epic: None A change in #105582 caused the regions/replicas query to return incorrect, unexpected results (instead of reducing the returned rows from the CTE to a single row, the cross join caused the number of rows to square, resulting in x^2 rows) causing the endpoint to never resolve properly, which in turn, caused an infinite loading state. This change fixes the regions/replicas query for the database details API. Release note (ui change): fix a broken query for the database details page that was causing an infinite loading state. Co-authored-by: Thomas Hardy <thardy@cockroachlabs.com>
This commit fixes a bug introduced in cockroachdb#105582 that caused SemaRejectFlags to be restored during semantic analysis, preventing the analysis from detecting some forms of invalid expressions. Fixes cockroachdb#108166 There is no release note because the related bug does not exist in any releases. Release note: None
This commit fixes a bug introduced in cockroachdb#105582 that caused SemaRejectFlags to be restored during semantic analysis, preventing the analysis from detecting some forms of invalid expressions. Fixes cockroachdb#108166 There is no release note because the related bug does not exist in any releases. 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>
A change in cockroachdb#105582 caused the regions/replicas query to return incorrect, unexpected results causing the endpoint to never resolve properly, which in turn, caused an infinite loading state. This change fixes the regions/replicas query for the database details API. Release note (ui change): fix a broken query for the database details page that was causing an infinite loading state.
opt: check that generator functions are not used in CASE or COALESCE
This patch adds checks during type-checking to ensure that generator functions
are not used in the arguments of
CASE,IF,COALESCE, orIFNULLexpressions. This mirrors postgres behavior. This patch also corrects the error
message that is returned in these cases to say "set-returning" instead of
"generator".
Fixes #97119
Fixes #94890
Release note (bug fix): CASE, IF, COALESCE, and IFNULL expressions now return
an error when passed a generator function as an argument. This mirrors postgres
behavior.