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: implement RETURNS TABLE syntax #137251

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Conversation

DrewKimball
Copy link
Collaborator

sql: do not limit set-returning UDF when it has OUT parameters

This commit fixes a bug that caused the SetOf option for the UDF
ReturnType to be overwritten if the UDF had OUT parameters. The bug
caused a LIMIT 1 to be imposed on the UDF's final body statement, so
that the UDF returned only a single row.

Fixes #128403

Release note (bug fix): Fixed a bug existing since v24.1 that would
cause a set-returning UDF with OUT parameters to return a single row.

sql: implement RETURNS TABLE syntax

This commit implements RETURNS TABLE for UDFs. RETURNS TABLE is
syntactic sugar for RETURNS SETOF with:

  • RECORD if there are multiple TABLE parameters, or
  • the type of the single TABLE parameter.
    The TABLE parameters are added to the list of routine parameters.

Fixes #100226

Release note (sql change): Added support for RETURNS TABLE syntax when
creating a UDF.

@DrewKimball DrewKimball requested a review from mgartner December 11, 2024 10:42
@DrewKimball DrewKimball requested review from a team as code owners December 11, 2024 10:42
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rytaft
Copy link
Collaborator

rytaft commented Dec 11, 2024

Do you plan to backport just the first commit? Or both?

@DrewKimball
Copy link
Collaborator Author

Do you plan to backport just the first commit? Or both?

Just the first commit.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Nice! Just some nits from me. :lgtm:

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


pkg/sql/logictest/testdata/logic_test/udf_setof line 246 at r2 (raw file):

subtest returns_table

statement error pgcode 42601 pq: at or near "EOF": syntax error: OUT and INOUT arguments aren't allowed in TABLE functions

nit: it seems a bit dirty that we have at or near "EOF": syntax error: prefix in the error message - the statement is parsable. Should we do this check after parsing for a cleaner message?


pkg/sql/opt/optbuilder/create_function.go line 310 at r1 (raw file):

			// errors in shared logic later.
			funcReturnType = types.Void
			cf.ReturnType = &tree.RoutineReturnType{

nit: perhaps add an test-only assertion that for procedures SetOf is not set?


pkg/sql/parser/sql.y line 4892 at r2 (raw file):

    // - RECORD if there are multiple TABLE parameters, or
    // - the type of the single TABLE parameter.
    // The TABLE parameters are

nit: the comment seems incomplete.

Copy link
Collaborator Author

@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.

TFTR!

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


pkg/sql/opt/optbuilder/create_function.go line 310 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps add an test-only assertion that for procedures SetOf is not set?

For procedures, ReturnType isn't set at all until this point.


pkg/sql/parser/sql.y line 4892 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: the comment seems incomplete.

Done.


pkg/sql/logictest/testdata/logic_test/udf_setof line 246 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: it seems a bit dirty that we have at or near "EOF": syntax error: prefix in the error message - the statement is parsable. Should we do this check after parsing for a cleaner message?

Done. It's definitely easiest to emit the error while parsing, because we parse functions in multiple places, and it means we don't have to track table function parameters separately in the AST. I added lexer.setErrNoDetails for cases like this where we don't want to annotate the error beyond giving it the Syntax error code.

This commit fixes a bug that caused the `SetOf` option for the UDF
`ReturnType` to be overwritten if the UDF had OUT parameters. The bug
caused a `LIMIT 1` to be imposed on the UDF's final body statement, so
that the UDF returned only a single row.

Fixes cockroachdb#128403

Release note (bug fix): Fixed a bug existing since v24.1 that would
cause a set-returning UDF with OUT parameters to return a single row.
This commit implements `RETURNS TABLE` for UDFs. `RETURNS TABLE` is
syntactic sugar for `RETURNS SETOF` with:
- RECORD if there are multiple TABLE parameters, or
- the type of the single TABLE parameter.
The TABLE parameters are added to the list of routine parameters.

Fixes cockroachdb#100226

Release note (sql change): Added support for `RETURNS TABLE` syntax when
creating a UDF.
@DrewKimball
Copy link
Collaborator Author

TFTR!

bors r+

craig bot pushed a commit that referenced this pull request Dec 12, 2024
137251: sql: implement RETURNS TABLE syntax r=DrewKimball a=DrewKimball

#### sql: do not limit set-returning UDF when it has OUT parameters

This commit fixes a bug that caused the `SetOf` option for the UDF
`ReturnType` to be overwritten if the UDF had OUT parameters. The bug
caused a `LIMIT 1` to be imposed on the UDF's final body statement, so
that the UDF returned only a single row.

Fixes #128403

Release note (bug fix): Fixed a bug existing since v24.1 that would
cause a set-returning UDF with OUT parameters to return a single row.

#### sql: implement RETURNS TABLE syntax

This commit implements `RETURNS TABLE` for UDFs. `RETURNS TABLE` is
syntactic sugar for `RETURNS SETOF` with:
- RECORD if there are multiple TABLE parameters, or
- the type of the single TABLE parameter.
The TABLE parameters are added to the list of routine parameters.

Fixes #100226

Release note (sql change): Added support for `RETURNS TABLE` syntax when
creating a UDF.

Co-authored-by: Drew Kimball <[email protected]>
@craig
Copy link
Contributor

craig bot commented Dec 12, 2024

Build failed:

@DrewKimball
Copy link
Collaborator Author

bors retry

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