refactor(udf): move UDF implementations to `expr_impl` #16759

wangrunji0408 · 2024-05-15T02:26:04Z

Signed-off-by: Runji Wang [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR moves UDF implementations from expr_core to expr_impl and introduces a distributed_slice in expr_core to collect them at link time. It makes it easier for developers to add new UDF languages. It also moves a lot dependencies out of expr_core, which can reduce build time and improve DX.

This PR makes each UDF a feature and they are not enabled by default to avoid building a lot of dependencies. For those who need to develop UDF, please enable them from ./risedev configure.

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

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: Runji Wang <[email protected]>

xxchan

Others generally LGTM. This also makes expr_udf cleaner, and maybe makes it easier to add new UDF implementations. Cool!

src/tests/simulation/Cargo.toml

src/frontend/src/handler/create_function.rs

ci/scripts/release.sh

xxchan · 2024-05-16T13:10:37Z

src/cmd_all/Cargo.toml

+default = ["rw-static-link"]
 rw-static-link = ["workspace-config/rw-static-link"]
 rw-dynamic-link = ["workspace-config/rw-dynamic-link"]
-embedded-deno-udf = ["risingwave_expr/embedded-deno-udf"]
-embedded-python-udf = ["risingwave_expr/embedded-python-udf"]
-default = ["rw-static-link"]
+all-udf = ["external-udf", "wasm-udf", "js-udf", "deno-udf", "python-udf"]
+external-udf = ["risingwave_expr_impl/external-udf"]
+wasm-udf = ["risingwave_expr_impl/wasm-udf"]
+js-udf = ["risingwave_expr_impl/js-udf"]
+deno-udf = ["risingwave_expr_impl/deno-udf"]
+python-udf = ["risingwave_expr_impl/python-udf"]


TBO I don't very like feature flags, but acceptable since there are already existing ones. 1 flag is as bad as N flags...

My consideration is that each UDF backend brings many dependencies, and most of our developers don't touch UDF at all. So it would be better to skip them in local development. The same thing could be applied to connectors. For example, I almost never use any connector in my development. Those dependencies could be skipped as well to save my disk.

BTW, I think it's time to split connector implementations off and design a clear interface for it, as there are now community developers willing to develop new connectors. 🤔

BTW, I think it's time to split connector implementations off

BTW, #12981 🤔

Yeah I noticed this PR. It's a little pity it was not merged, but I think it's in the right direction.

src/expr/core/src/expr/expr_udf.rs

src/expr/core/src/sig/mod.rs

xxchan · 2024-05-16T14:06:37Z

src/expr/core/src/expr/expr_udf.rs

+#[derive(Debug, Clone)]
+struct MetricsVec {
+    /// Number of successful UDF calls.
+    success_count: IntCounterVec,


Better to use LabelGuardedIntCounterVec, etc. #14838

let's do it in the following PR.

src/expr/core/src/sig/mod.rs

Signed-off-by: Runji Wang <[email protected]>

BugenZhao

Rest looks amazing 🤩

BugenZhao · 2024-05-20T05:05:46Z

ci/scripts/release.sh

@@ -71,8 +71,8 @@ if [ "${ARCH}" == "aarch64" ]; then
  # see https://github.com/tikv/jemallocator/blob/802969384ae0c581255f3375ee2ba774c8d2a754/jemalloc-sys/build.rs#L218
  export JEMALLOC_SYS_WITH_LG_PAGE=16
 fi
-cargo build -p risingwave_cmd_all --features "rw-static-link" --profile release
-cargo build -p risingwave_cmd --bin risectl --features "rw-static-link" --profile release
+cargo build -p risingwave_cmd_all --features "rw-static-link" --features all-udf --profile release


Interesting. Does it mean that we used to release versions without support for embedded
UDF?

I'm considering whether we should make it a default feature but always use --no-default-features for RiseDev builds, so that we can avoid such mistakes in the future...

Oh yes, the released version did not contain python UDF and deno UDF.

But making these features as default will cause them to be built by rust-analyzer...

I'm considering whether we should make it a default feature but always use --no-default-features for RiseDev builds, so that we can avoid such mistakes in the future...

I don't think it's worth it:

When release failed, we can just fix it. And it's ok if the script is dirty. We just need to get it done right once.

But default features is to make developers happy, which can be hard to fix.

But making these features as default will cause them to be built by rust-analyzer...

Can also suggest developers to set rust-analyzer.cargo.noDefaultFeatures in the editor. 😕

But default features is to make developers happy, which can be hard to fix.

The UDF module is touched by few people, so it makes sense to require manual enabling on the features.

However, imagine we're going to add conditional compiles for the connector module, which is actively developed and maintained by a considerable amount of developers. We still need the effort to inform them how to enable these features, especially the IDE part, which is not better than the approach above.

The point is the most of developers do not pay much attention to the compile time. If something comes naturally, it's good. If it requires extra effort, they might give up. Thus in my opinion, it's fine to treat it as a hidden treasure needed to be discovered by some manual efforts. 😄

The point is the most of developers do not pay much attention to the compile time. If something comes naturally, it's good. If it requires extra effort, they might give up.

I 100% agree. However, my opinion from this fact is that it's better to offer "free" fast compile time for developers. If they are not good at it, we'd better make the default faster. Saving every one's time does good to the whole world!

how to enable these features, especially the IDE part

I'm thinking whether it's possible to offer IDE experience without config for feature flags. e.g., cfg_or_panic(madsim).

One idea is that: we put each connector impl in separated crate, which is an optional dependency. If so, maybe the IDE can work without config.

Let's go to #16841

src/expr/core/src/sig/udf.rs

Signed-off-by: Runji Wang <[email protected]>

TennyZhuang

LGTM

wangrunji0408 requested review from BugenZhao and xxchan May 15, 2024 02:26

wangrunji0408 requested a review from a team as a code owner May 15, 2024 02:26

github-actions bot added the type/refactor label May 15, 2024

wangrunji0408 added 4 commits May 15, 2024 16:35

introduce UDF runtime interface

2f91a28

Signed-off-by: Runji Wang <[email protected]>

replace FunctionRuntime by Ident in parser

7dffa28

Signed-off-by: Runji Wang <[email protected]>

move creation code from frontend to expr_impl

882d8b4

Signed-off-by: Runji Wang <[email protected]>

make wasm udf optional

dc71c16

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 force-pushed the wrj/dyn-udf-runtime branch from 85a3a41 to dc71c16 Compare May 15, 2024 08:36

wangrunji0408 requested a review from TennyZhuang May 15, 2024 08:37

wangrunji0408 added 7 commits May 15, 2024 17:14

make every udf an optional feature

c6b1a15

Signed-off-by: Runji Wang <[email protected]>

add config to risedev

dc2e5ca

Signed-off-by: Runji Wang <[email protected]>

update error msg

0291a23

Signed-off-by: Runji Wang <[email protected]>

fix clippy

498a3d4

Signed-off-by: Runji Wang <[email protected]>

enable js udf on madsim

ed88714

Signed-off-by: Runji Wang <[email protected]>

fix clippy

6261b2a

Signed-off-by: Runji Wang <[email protected]>

fix building simulation

ef675c0

Signed-off-by: Runji Wang <[email protected]>

xxchan requested changes May 16, 2024

View reviewed changes

resolve comments

f13b617

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 requested a review from xxchan May 20, 2024 04:24

xxchan approved these changes May 20, 2024

View reviewed changes

BugenZhao reviewed May 20, 2024

View reviewed changes

return error when finding multiple udf impls

801ad00

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 added this pull request to the merge queue May 20, 2024

TennyZhuang approved these changes May 20, 2024

View reviewed changes

Merged via the queue into main with commit 8b66c91 May 20, 2024
27 of 28 checks passed

wangrunji0408 deleted the wrj/dyn-udf-runtime branch May 20, 2024 07:41

xxchan mentioned this pull request May 20, 2024

split connector implementations into crates #16841

Open

wangrunji0408 mentioned this pull request May 22, 2024

fix(build): disable python udf and deno udf on release #16887

Merged

9 tasks

BugenZhao mentioned this pull request Aug 2, 2024

checklist for releasing v1.11 (or v2.0) risingwavelabs/homebrew-risingwave#43

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(udf): move UDF implementations to `expr_impl` #16759

refactor(udf): move UDF implementations to `expr_impl` #16759

wangrunji0408 commented May 15, 2024 •

edited

Loading

xxchan left a comment

xxchan May 16, 2024

wangrunji0408 May 17, 2024

wangrunji0408 May 17, 2024

xxchan May 18, 2024

wangrunji0408 May 20, 2024

xxchan May 16, 2024

wangrunji0408 May 20, 2024

BugenZhao left a comment

BugenZhao May 20, 2024

BugenZhao May 20, 2024

wangrunji0408 May 20, 2024

wangrunji0408 May 20, 2024 •

edited

Loading

xxchan May 20, 2024

BugenZhao May 20, 2024

BugenZhao May 20, 2024

xxchan May 20, 2024

xxchan May 20, 2024

xxchan May 20, 2024

TennyZhuang left a comment

refactor(udf): move UDF implementations to expr_impl #16759

refactor(udf): move UDF implementations to expr_impl #16759

Conversation

wangrunji0408 commented May 15, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

xxchan left a 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

BugenZhao left a 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 May 20, 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

TennyZhuang left a comment

Choose a reason for hiding this comment

refactor(udf): move UDF implementations to `expr_impl` #16759

refactor(udf): move UDF implementations to `expr_impl` #16759

wangrunji0408 commented May 15, 2024 •

edited

Loading

wangrunji0408 May 20, 2024 •

edited

Loading