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

sql: fix UDF with VOID return type #108299

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Aug 7, 2023

Functions with VOID returns types should be allowed to produce any
number of columns of any type from their last statement. Prior to this
commit, a calling UDF with a VOID return type would error if the last
statement in the UDF did not produce a value of type VOID. This commit
fixes this minor bug.

Fixes #108297

Release note (bug fix): The last SQL statement in a user-defined
function with a VOID return type can now produce any number of columns
of any type. This bug was present since UDFs were introduced in version
22.2.

@mgartner mgartner requested a review from a team as a code owner August 7, 2023 16:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rharding6373 rharding6373 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 (waiting on @DrewKimball and @mgartner)


pkg/sql/opt/optbuilder/scalar.go line 736 at r1 (raw file):

		// column would be pruned and the contents of last statement would not
		// be executed.
		// TODO(mgartner): This will add some planning overhead for every

Would adding this statement in buildCreateFunction instead be more efficient? The main issue I see in that proposal is that we'd print the extra statement in show create function, unless we had some kind of annotation in place.

Alternatively, in cases where we don't inline the function, we could add some code to routine to execute all statements but return null at the end when the return type is void. I'm not sure what an alternative would be if we don't need to use routines, though, and I don't know if preventing inlining for void return types is worth it.


pkg/sql/logictest/testdata/logic_test/udf_regressions line 620 at r1 (raw file):

NULL

# Invoking the UDF above should have increment s108297 to 1, so calling nextval

Nice test.

Functions with VOID returns types should be allowed to produce any
number of columns of any type from their last statement. Prior to this
commit, a calling UDF with a VOID return type would error if the last
statement in the UDF did not produce a value of type VOID. This commit
fixes this minor bug.

Fixes cockroachdb#108297

Release note (bug fix): The last SQL statement in a user-defined
function with a VOID return type can now produce any number of columns
of any type. This bug was present since UDFs were introduced in version
22.2.
Copy link
Collaborator Author

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

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


pkg/sql/opt/optbuilder/scalar.go line 736 at r1 (raw file):

Would adding this statement in buildCreateFunction instead be more efficient? The main issue I see in that proposal is that we'd print the extra statement in show create function, unless we had some kind of annotation in place.

It'd make it harder to change our implementation of RETURNS VOID in future releases because we'd have to update existing UDFs to remove the extra statement.

Alternatively, in cases where we don't inline the function, we could add some code to routine to execute all statements but return null at the end when the return type is void. I'm not sure what an alternative would be if we don't need to use routines, though, and I don't know if preventing inlining for void return types is worth it.

Ya, maybe this special logic for RETURNS VOID should be during evaluation of the routine - then it would add no overhead. I'll try that in a separate PR.

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @rharding6373)


pkg/sql/opt/optbuilder/scalar.go line 769 at r1 (raw file):

			panic(err)
		}
		// TODO(#108298): Figure out how to handle PLpgSQL functions with VOID

We could also directly construct the Values expression, that would make it easier to handle PLpgSQL as well.

@mgartner
Copy link
Collaborator Author

mgartner commented Aug 7, 2023

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 7, 2023

Build succeeded:

@craig craig bot merged commit 8c1ff51 into cockroachdb:master Aug 7, 2023
@mgartner mgartner deleted the 108297-udf-void branch August 8, 2023 12:45
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.

sql: UDFs with VOID returns types do not work as expected
4 participants