feat(sql-udf): support basic anonymous sql udf #14139

xzhseh · 2023-12-21T23:58:39Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR adds basic support for anonymous sql udf, the current supported syntax is simple, and is a subset of pg's create function standard (refer: https://www.postgresql.org/docs/current/sql-createfunction.html), for all to-be-supported syntax in RW, check #10151 for details.

The current supported syntax (anonymous parameter list) is as follows:

single quote definition

create function sub(INT, INT) returns int language sql as 'select $1 - $2';

double dollar definition

create function add(INT, INT) returns int language sql as $$select $1 + $2$$;

return expression

create function add(INT, INT) returns int language sql return $1 + $2;

multiple types interleaving

create function add_sub(INT, FLOAT, INT) returns float language sql as $$select -$1 + $2 - $3$$;

calls other pre-defined sql udfs / built-in functions

create function add_sub_binding() returns int language sql as 'select add(1, 1) + sub(2, 2)';
create function add_sub_wrapper(INT, INT) returns int language sql as 'select add($1, $2) + sub($1, $2) + 114512';
create function call_regexp_replace() returns varchar language sql as $$select regexp_replace('💩💩💩💩💩foo🤔️bar亲爱的😭baz这不是爱情❤️‍🔥', 'baz(...)', '这是🥵', 'ic')$$;

Note: recursive definition is NOT supported, e.g.,

# Recursive definition is forbidden
statement error recursive definition is forbidden, please recheck your function syntax
create function recursive(INT, INT) returns int language sql as 'select recursive($1, $2) + recursive($1, $2)';

See e2e_test/udf/sql_udf.slt for detailed use cases.

P.S. The current implementation will only check for syntax correctness (i.e., the sql body in as clause) by utilizing Parser::parse_sql, semantic check will be implemented in the future.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

Support basic anonymous sql user defined function, the current supported syntax is limited. See the above documentation for details.

src/frontend/src/handler/create_sql_function.rs

src/frontend/src/handler/mod.rs

src/frontend/src/binder/expr/function.rs

wangrunji0408 · 2023-12-22T03:18:26Z

src/frontend/src/handler/create_sql_function.rs

+        database_id,
+        name: function_name,
+        kind: Some(kind),
+        arg_types: arg_types.into_iter().map(|t| t.into()).collect(),


Looks like the type of this field should be changed to Vec<Field> or Schema so that we can find argument names here.

Will change this field after reviewing the anonymous implementation logic

src/frontend/src/binder/expr/function.rs

wangrunji0408 · 2023-12-22T03:44:06Z

P.S. the current parsing will be triggered per bind_function call which is super inefficient, and this will be refactored later.

Actually I think it is acceptable to parse the function every time it is bound. In order to be efficient, we can cache the parsed AST in the binder in case there are multiple calls in a query.

I prefer to store the original string of function body rather than any parsed form.

wangrunji0408

Rest LGTM. Good work!

src/frontend/src/session.rs

src/frontend/src/handler/mod.rs

src/frontend/src/catalog/schema_catalog.rs

src/frontend/src/binder/expr/column.rs

src/frontend/src/binder/expr/mod.rs

src/frontend/src/binder/expr/function.rs

wangrunji0408 · 2023-12-28T04:31:07Z

src/frontend/src/binder/expr/function.rs

+                create_udf_context(self, &args, &Arc::clone(func));
+                return self.bind_expr(extract_udf_expression(ast));


I'm not sure if we should create a new binder to do the inline. Or at least we should clean the udf_context after inlining, otherwise the following binding will be affected.

I prefer the latter solution, creating a new binder to do the inline may add extra cost and is hard to integrate with the on-going binding process in the main routine 🤔️?

src/frontend/src/binder/expr/function.rs

xxchan

Cool!

Some comments for missing tests. Generally, I expect there are test covering all errors. They can also kind of function as the specification.

src/sqlparser/tests/sqlparser_postgres.rs

e2e_test/udf/sql_udf.slt

src/frontend/src/handler/mod.rs

e2e_test/udf/sql_udf.slt

src/frontend/src/catalog/root_catalog.rs

xxchan · 2023-12-28T12:37:45Z

src/frontend/src/handler/mod.rs

src/frontend/src/binder/expr/column.rs

)

github-actions

license-eye has totally checked 4688 files.

Valid	Invalid	Ignored	Fixed
2048	1	2639	0

Click to see the invalid file list

src/frontend/src/handler/create_sql_function.rs

src/frontend/src/handler/create_sql_function.rs

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

chenzl25

Is there any way for us to view the definition of SQL UDFs? show functions and show create function xxx seems can't.

xzhseh · 2024-01-02T04:30:58Z

show functions and show create function xxx seems can't.

show functions works fine (without definition), but show create function xxx is not supported.
And since the definitions of sql udfs are also stored in function catalog, it's easy to implement such feature if needed. (I can come up with a PR that supports this)

wangrunji0408 · 2024-01-02T06:05:34Z

e2e_test/udf/sql_udf.slt

+statement ok
+create function call_regexp_replace() returns varchar language sql as $$select regexp_replace('💩💩💩💩💩foo🤔️bar亲爱的😭baz这不是爱情❤️‍🔥', 'baz(...)', '这是🥵', 'ic')$$;
+
+statement error Expected end of statement, found: 💩
+create function call_regexp_replace() returns varchar language sql as 'select regexp_replace('💩💩💩💩💩foo🤔️bar亲爱的😭baz这不是爱情❤️‍🔥', 'baz(...)', '这是🥵', 'ic')';


stdrc · 2024-01-02T06:14:44Z

src/frontend/src/handler/create_sql_function.rs

+    // We do NOT allow recursive calling inside sql udf
+    // Since there does not exist the base case for this definition
+    if body.contains(format!("{}(", name.real_value()).as_str()) {
+        return Err(ErrorCode::InvalidInputSyntax(
+            "recursive definition is forbidden, please recheck your function syntax".to_string(),
+        )
+        .into());
+    }


What if create function recursive(INT, INT) returns int language sql as 'select not_recursive($1, $2) + not_recursive($1, $2)';?

Or create function foo(INT) returns int language sql as 'select bar($1)'; create function bar(INT) returns int language sql as 'select foo($1)';?

What if create function foo(INT) returns varchar language sql as $$select 'foo(INT)'$$;?

After testing on my local pg server, it seems that pg leaves this problem to users 😅, and for select not_recursive($1, $2) we can use a regexp to exact match the pattern, but I can't come up with a graceful way for the nested calling recursive chain. (And pg does not support this too)

So should we handle this gracefully for the users, or just leave it to them just as what pg does?

P.S. RW will quit after stack overflow

What if create function foo(INT) returns varchar language sql as $$select 'foo(INT)'$$;?

Indeed, under current implementation this definition will be rejected though it's totally valid, but it will definitely add complexity to the codebase if we add check conditions for this.
The problem remains: should we handle this for users?

but I can't come up with a graceful way for the nested calling recursive chain.

we can also have a max_recursive_depth to prevent infinite recursion.

Currently there are two solutions (by @wangrunji0408) to this problem,

Simulate pg's behavior, add a max_recursive_depth field somewhere to prevent infinite recursion.

Add a field to binder indicating which functions are currently being inlined and should not be called again.

Which one do you prefer? cc @xxchan @stdrc @BugenZhao.

I prefer solution 1.

A little side note: RC's case (mutual recursion) will be rejected by PG due to function does not exist😄

Wait a minute. Why we can't do recursive check here? Shouldn't it also fail with "function does not exist", similar with mutual recursion?

will be rejected by PG due to function does not exist

Good catch!
Currently we don't have the ability to check if the inner calling functions exist during definition time, the problem will only be exposed when actual calling the function. (i.e., during binding phase)
I prefer to support the checking in later PRs.

src/frontend/src/handler/mod.rs

src/frontend/src/handler/create_sql_function.rs

wangrunji0408 · 2024-01-02T07:14:24Z

src/frontend/src/binder/expr/function.rs

+                    // If the `udf_context` is not empty, this means the current binding
+                    // function is not the root binding sql udf, thus we should NOT
+                    // clean the context after binding.
+                    clean_flag = false;


I'm feeling that it's time to create a new binder to do recursive scoped binding, which could simplify state management. 🤔

Maybe add an extra field in binder, e.g., udf_scope_binder: Option<Binder>?
But I'm worrying the extra cost of adding a new binder when using sql udf, it should be acceptable though 🤣

No, I mean creating a new binder on the stack (with only function parameters as available identifiers) and calling the bind method. This could resolve the name conflict problem.
The cost can be ignored in the binding stage I think. 😄

Will put this to later PRs. 🤡

xxchan · 2024-01-02T08:24:11Z

e2e_test/udf/sql_udf.slt

QQ: Can prepared statements work correctly? And are there any tests?

It might be surprising but I couldn't find any e2e tests for the prepared statement itself. 😕

#14141

Can prepared statements work correctly?

I think so, under current implementation the check of udf_context will be immediately cleared after bind_function, which ensure it will not interleave with the parameter bindings of prepare-related statements. (e.g., triggered by third party drivers, etc)

xzhseh · 2024-01-02T09:39:36Z

Below is the list for future to-dos, just for reference:

semantic check at definition time (e.g., return type mismatch, inner functions' existence, etc)
introduce a runtime binder to reduce manual state management
parameterized sql udf
recursive prevention, reference: feat(sql-udf): support basic anonymous sql udf #14139 (comment)
Anyways, this PR is ready to merge.

wangrunji0408

LGTM! Thanks!

xxchan · 2024-01-07T05:59:07Z

btw why is this called “ anonymous”?

xzhseh · 2024-01-07T11:24:52Z

btw why is this called “ anonymous”?

The parameters have no name. (i.e., $1 / $2 / ..., so I name it anonymous 😄)

TennyZhuang · 2024-01-08T03:09:22Z

In risingwave even building functions parameters also don’t have names because we don’t support named parameters yet

wangrunji0408 · 2024-01-08T04:45:21Z

In writing wave even building functions parameters also don’t have names because we don’t support named parameters yet

Should we support it? It can facilitate some function callings, such as:

jsonb_path_query(jsonb, jsonpath, silent => true);

hengm3467 · 2024-01-22T01:05:40Z

Making sure I understand it correctly. We now have SQL-UDF in parralel with external UDFs (Javan and Python), which requires deploying additional servers/instances. Correct? Thanks. @wangrunji0408 @BugenZhao

xzhseh · 2024-01-22T03:21:22Z

We now have SQL-UDF in parralel with external UDFs (Javan and Python), which requires deploying additional servers/instances.

Exactly. And sql udf does not rely on external service, basically performs inlining logic, cc @hengm3467.

feat(sql-udf): support basic sql udf function

6e3f5d4

xzhseh self-assigned this Dec 21, 2023

xzhseh requested review from wangrunji0408, TennyZhuang and xxchan December 21, 2023 23:58

github-actions bot added the type/feature label Dec 22, 2023

This comment was marked as outdated.

Sign in to view

github-actions bot added the breaking-change label Dec 22, 2023

This comment was marked as resolved.

Sign in to view

xzhseh removed the breaking-change label Dec 22, 2023

wangrunji0408 reviewed Dec 22, 2023

View reviewed changes

xzhseh added 3 commits December 25, 2023 19:35

support anonymous sql udf

bf57014

remove redundant code

16780b4

add basic test for anonymous sql udf

fa8fc7a

xzhseh changed the title ~~feat(sql-udf): support basic sql udf function~~ feat(sql-udf): support basic anonymous sql udf Dec 26, 2023

xzhseh marked this pull request as ready for review December 26, 2023 22:17

xzhseh requested review from wangrunji0408 and xxchan December 26, 2023 22:17

xzhseh added 2 commits December 26, 2023 17:52

add more test cases for parser & sql_udf

5b5c82d

Merge branch 'main' into xzhseh/sql-udf

456d6fd

xiangjinwu mentioned this pull request Dec 28, 2023

feat(sql-udf): support basic anonymous sql udf return expression #14245

Merged

9 tasks

wangrunji0408 reviewed Dec 28, 2023

View reviewed changes

src/frontend/src/binder/expr/function.rs Outdated Show resolved Hide resolved

xxchan reviewed Dec 28, 2023

View reviewed changes

src/frontend/src/handler/mod.rs Show resolved Hide resolved

xxchan reviewed Dec 28, 2023

View reviewed changes

src/frontend/src/binder/expr/column.rs Show resolved Hide resolved

xzhseh added 2 commits December 28, 2023 15:27

feat(sql-udf): support basic anonymous sql udf return expression (#14245

94ebbda

)

Merge branch 'main' into xzhseh/sql-udf

22ca504

Merge branch 'main' into xzhseh/sql-udf

9ccaa07

github-actions bot reviewed Jan 2, 2024

View reviewed changes

src/frontend/src/handler/create_sql_function.rs Outdated Show resolved Hide resolved

xzhseh and others added 2 commits January 1, 2024 22:15

update copyright

c6d677b

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

update copyright

88b00f1

chenzl25 reviewed Jan 2, 2024

View reviewed changes

wangrunji0408 reviewed Jan 2, 2024

View reviewed changes

stdrc reviewed Jan 2, 2024

View reviewed changes

wangrunji0408 reviewed Jan 2, 2024

View reviewed changes

remove unnecessary check & refactor

53e53ff

xxchan reviewed Jan 2, 2024

View reviewed changes

xzhseh mentioned this pull request Jan 2, 2024

feat(udf): add body field for udf body definition #14300

Merged

9 tasks

feat(udf): add body field for udf body definition (#14300)

d6c3d0d

xzhseh added 2 commits January 2, 2024 04:45

Merge branch 'main' into xzhseh/sql-udf

199f04f

Merge branch 'main' into xzhseh/sql-udf

d601ac8

xzhseh requested a review from wangrunji0408 January 4, 2024 06:49

wangrunji0408 approved these changes Jan 5, 2024

View reviewed changes

xzhseh added this pull request to the merge queue Jan 5, 2024

Merged via the queue into main with commit 75735e2 Jan 5, 2024
28 checks passed

xzhseh deleted the xzhseh/sql-udf branch January 5, 2024 08:34

xzhseh mentioned this pull request Jan 5, 2024

feat(sql-udf): deep calling stack (recursion) prevention for sql udf #14392

Merged

9 tasks

xzhseh mentioned this pull request Jan 9, 2024

chore(sql-udf): update test cases for implicit type cast support #14458

Merged

9 tasks

		create_udf_context(self, &args, &Arc::clone(func));
		return self.bind_expr(extract_udf_expression(ast));

feat(sql-udf): support basic anonymous sql udf #14139

feat(sql-udf): support basic anonymous sql udf #14139

Conversation

xzhseh commented Dec 21, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

This comment was marked as outdated.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangrunji0408 commented Dec 22, 2023

wangrunji0408 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

xxchan commented Dec 28, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

xzhseh commented Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhseh Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangrunji0408 Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

xzhseh Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

xzhseh commented Jan 2, 2024 • edited Loading

wangrunji0408 left a comment

Choose a reason for hiding this comment

xxchan commented Jan 7, 2024

xzhseh commented Jan 7, 2024

TennyZhuang commented Jan 8, 2024 • edited Loading

wangrunji0408 commented Jan 8, 2024

hengm3467 commented Jan 22, 2024

xzhseh commented Jan 22, 2024 • edited Loading

xzhseh commented Dec 21, 2023 •

edited

Loading

xzhseh commented Jan 2, 2024 •

edited

Loading

xzhseh Jan 2, 2024 •

edited

Loading

wangrunji0408 Jan 2, 2024 •

edited

Loading

BugenZhao Jan 2, 2024 •

edited

Loading

xzhseh Jan 2, 2024 •

edited

Loading

xzhseh commented Jan 2, 2024 •

edited

Loading

TennyZhuang commented Jan 8, 2024 •

edited

Loading

xzhseh commented Jan 22, 2024 •

edited

Loading