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.

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

opt: add FoldFunctionWithNullArg normalization rule #62924

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Apr 1, 2021

This commit adds the FoldFunctionWithNullArg normalization rule which
folds a function that does not allow Null arguments to Null when one of
the arguments is Null. See the documentation for the new rule for more
details.

Release note (performance improvement): The optimizer now folds
functions to NULL when the function does not allow NULL arguments
and one of the arguments is a NULL constant. As a result, more
efficient query plans will be produced for queries with these types of
function calls.

@mgartner mgartner requested a review from a team as a code owner April 1, 2021 01:46
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner
Copy link
Collaborator Author

mgartner commented Apr 1, 2021

The first commit is from #62893.

@mgartner
Copy link
Collaborator Author

mgartner commented Apr 1, 2021

This might be proof that we can fold ANY function where NullableArgs=false and one of the args is Null:

if arg == DNull && !expr.fnProps.NullableArgs {

It looks like a function is never really evaluated in that case. So maybe I can lift the restrictions on the function being non-volatile and a normal function?

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

To decide whether to support volatile and non-normal functions, maybe check what the TypeCheck code does for non-typed NULLs? Might also be useful to get the perspective from @yuzefovich on the execution side. But it seems to me like at least volatile functions should be safe to fold here. Not 100% sure about non-normal functions.

Reviewed 3 of 3 files at r1, 5 of 5 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)

@RaduBerinde
Copy link
Member

LGTM

In principle, we can have a volatile function that has some side-effect, even if it always returns NULL on NULL input. But I don't think we have any real cases like that, and as you pointed out, the existing code wouldn't evaluate it anyway:

demo@127.0.0.1:26257/defaultdb> select crdb_internal.force_error('', 'foo');
ERROR: foo
demo@127.0.0.1:26257/defaultdb> select crdb_internal.force_error('', NULL);
  crdb_internal.force_error
-----------------------------
  NULL
(1 row)

Let's add to the documentation for FunctionProperties.NullableArgs saying explicitly that the function won't be evaluated if an arg is NULL so if it is expected to produce side-effects in that case, the flag should be true (and that if this is changed, this optimizer rule should be adjusted).

@mgartner
Copy link
Collaborator Author

mgartner commented Apr 1, 2021

Let's add to the documentation for FunctionProperties.NullableArgs saying explicitly that the function won't be evaluated if an arg is NULL so if it is expected to produce side-effects in that case, the flag should be true (and that if this is changed, this optimizer rule should be adjusted).

Good idea. Done.

This commit adds the `FoldFunctionWithNullArg` normalization rule which
folds a function that does not allow Null arguments to Null when one of
the arguments is Null. See the documentation for the new rule for more
details.

Release note (performance improvement): The optimizer now folds
functions to `NULL` when the function does not allow `NULL` arguments
and one of the arguments is a `NULL` constant. As a result, more
efficient query plans will be produced for queries with these types of
function calls.
@mgartner
Copy link
Collaborator Author

mgartner commented Apr 6, 2021

@RaduBerinde mind taking a final look before I merge?

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)

@mgartner
Copy link
Collaborator Author

mgartner commented Apr 6, 2021

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 6, 2021

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants