-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
release-23.1.0: opt: fix strict UDFs with subquery arguments #101951
release-23.1.0: opt: fix strict UDFs with subquery arguments #101951
Conversation
Previously, we wrapped all strict UDFs with a `CASE` expression to prevent the UDF from being invoked when any of the arguments were `NULL`. This was required in order to inline strict UDFs (see #94797). Unfortunately, wrapping the arguments in a `CASE` expression caused problems when the arguments were subqueries. The subqueries would be duplicated in the optimizer expression, one copy used in the `CASE` expression and another as an argument to the UDF`, with each copy containing the same table and column IDs. Normalization rules could then transform these subqueries into apply-joins that would have intersecting columns in their left and right children. This caused optimizer assertion failures in debug builds and could cause incorrect results in release builds. This commit implements a temporary fix which removes the synthesized `CASE` expression entirely. The logic for returning `NULL` immediately when a strict UDF is invoked with a `NULL` argument has been added back into `planner.EvalRoutineExpr`, and strict UDFs are not longer inlined. We should be able to inline strict UDFs by adding the `CASE` expression during the `InlineUDF` normalization rule, as long as the arguments are not complex expressions (e.g., only variables, constants, or placeholders). This is left for a future commit. Fixes #101516 Release note (bug fix): A bug has been fixed that could cause incorrect results for queries invoking `STRICT` user-defined functions. This bug was only present in pre-release versions of 23.1.
Release note: None
The description of ExprFmtFlags has been simplified. It previously contained a very detailed description of bitmasks and iota, which probably isn't warranted because it is such a common pattern in the codebase.
This commit inlines strict UDFs by wrapping the generated subquery expression with a `CASE` expression. The `CASE` expression results in `NULL` if any of the arguments of the UDF are `NULL`. Release note: None
568b675
to
9de3ab1
Compare
0d4731a
to
bc6ac49
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, @msirek, and @rharding6373)
Backport 4/4 commits from #101867 on behalf of @mgartner.
/cc @cockroachdb/release
opt: fix strict UDFs with subquery arguments
Previously, we wrapped all strict UDFs with a
CASE
expression toprevent the UDF from being invoked when any of the arguments were
NULL
. This was required in order to inline strict UDFs (see #94797).Unfortunately, wrapping the arguments in a
CASE
expression causedproblems when the arguments were subqueries. The subqueries would be
duplicated in the optimizer expression, one copy used in the
CASE
expression and another as an argument to the UDF`, with each copy
containing the same table and column IDs. Normalization rules could then
transform these subqueries into apply-joins that would have intersecting
columns in their left and right children. This caused optimizer
assertion failures in debug builds and could cause incorrect results in
release builds.
This commit implements a temporary fix which removes the synthesized
CASE
expression entirely. The logic for returningNULL
immediatelywhen a strict UDF is invoked with a
NULL
argument has been added backinto
planner.EvalRoutineExpr
, and strict UDFs are not longer inlined.We should be able to inline strict UDFs by adding the
CASE
expressionduring the
InlineUDF
normalization rule, as long as the arguments arenot complex expressions (e.g., only variables, constants, or
placeholders). This is left for a future commit.
Fixes #101516
Release note (bug fix): A bug has been fixed that could cause incorrect
results for queries invoking
STRICT
user-defined functions. This bugwas only present in pre-release versions of 23.1.
opt: add strict field to UDF expression format
Release note: None
opt: simplify description of ExprFmtFlags
The description of ExprFmtFlags has been simplified. It previously
contained a very detailed description of bitmasks and iota, which
probably isn't warranted because it is such a common pattern in the
codebase.
opt: inline strict UDFs
This commit inlines strict UDFs by wrapping the generated subquery
expression with a
CASE
expression. TheCASE
expression results inNULL
if any of the arguments of the UDF areNULL
.Note that even though the subquery is wrapped in a
CASE
expression, itmay still be evaluated due to the fact that non-correlated subqueries
are eagerly evaluated, see #20298. This should be fine because these
inlined UDFs shouldn't have side-effects - we don't inline volatile
UDFs.
Release note: None
Release justification: Fixes a UDF regression.