feat(meta): dependency check for `drop function` #19399

stdrc · 2024-11-15T06:11:26Z

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

What's changed and what's your intention?

This PR introduces dependencies field in CreateMaterializedViewRequest and CreateSinkRequest, and records dependencies upon UDFs uniformly with dependencies upon relations through this field.

Fixes #17263.

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.

stdrc · 2024-11-15T06:11:41Z

feat(meta): dependency check for drop function #19399 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

e2e_test/slow_tests/udf/always_retry_python.slt

xxchan

Generally LGTM

xxchan · 2024-11-15T09:18:01Z

src/meta/src/manager/streaming_job.rs

+    // TODO: to be removed, pass all objects uniformly through `dependencies` field instead.
    pub fn dependent_relations(&self) -> Vec<u32> {


It seems table.dependent_relations is not set any more. Will it break sth to call this function?

Oh. The caller uses extend

I would be in favor of removing this field together now to avoid mistakes. Can put in an upstack PR and merge them together.

It seems table.dependent_relations is not set any more. Will it break sth to call this function?

I merged the result of this method and the new dependencies field to avoid breaking. And now it basically return empty vector for MaterializedView and Sink, and remain the same for Table and Index.

Since this PR is mainly for drop function check, I only changed the behavior of MaterializedView and Sink. Added TODO comments for the remaining ones.

src/meta/src/controller/streaming_job.rs

e2e_test/udf/drop_function.slt

proto/ddl_service.proto

BugenZhao · 2024-11-18T04:12:10Z

proto/ddl_service.proto

@@ -86,6 +86,8 @@ message CreateSinkRequest {
  stream_plan.StreamFragmentGraph fragment_graph = 2;
  // It is used to provide a replace plan for the downstream table in `create sink into table` requests.
  optional ReplaceTablePlan affected_table_change = 3;
+  // The list of object IDs that this materialized view depends on.
+  repeated uint32 dependencies = 4;


Is it accurately direct dependencies? I suppose the meta service can recursively resolve all dependencies.

BugenZhao · 2024-11-18T04:13:02Z

src/connector/src/sink/catalog/desc.rs

@@ -104,7 +103,7 @@ impl SinkDesc {
            downstream_pk: self.downstream_pk,
            distribution_key: self.distribution_key,
            owner,
-            dependent_relations,
+            dependent_relations: vec![], // TODO(rc): to be deprecated


Are we going to deprecate all dependent_relations fields in different catalogs? Are they referenced anywhere?

I don't think these dependent_relations fields are used elsewhere except when creating meta objects. cc @yezizp2012 can you confirm?

IIUC that's the only usage when creating meta objects. Previouly the process of detecting dependency cycles for sink into table relies on it in v1, but that functionality has been transferred to the meta side. However, the code related to this in the FE remains and can be removed for clarity.

FYI, @shanicky will rise a PR to clean that part.

src/frontend/src/handler/create_mv.rs

yezizp2012

LGTM! Thx for the PR. Please help to unify the behaviors for other relations like index, source as well.
Additionally, following this PR, we can provide syntax support for granting and revoking function privileges to enhance user experience.

yezizp2012 · 2024-11-19T06:17:09Z

src/meta/service/src/ddl_service.rs

@@ -241,6 +242,7 @@ impl DdlService for DdlServiceImpl {
                        fragment_graph,
                        CreateType::Foreground,
                        None,
+                        HashSet::new(), // TODO(rc): pass dependencies through this field instead of `PbSource`


👍 connection and secret are handled in separated fields now and can be unified into dependencies. This also applies to the connection for the sink.

…a clone Signed-off-by: Richard Chien <[email protected]>

Signed-off-by: Richard Chien <[email protected]>

Co-authored-by: Bugen Zhao <[email protected]>

Signed-off-by: Richard Chien <[email protected]>

stdrc changed the title ~~change Binder::included_relations() to return reference instead of a clone~~ feat(meta): dependency check for function Nov 15, 2024

stdrc changed the title ~~feat(meta): dependency check for function~~ feat(meta): dependency check for drop function Nov 15, 2024

github-actions bot added type/feature Invalid PR Title ci/run-e2e-single-node-tests ci/run-e2e-test-other-backends labels Nov 15, 2024

stdrc marked this pull request as ready for review November 15, 2024 07:13

stdrc requested review from fuyufjh, yezizp2012 and BugenZhao November 15, 2024 07:17

github-actions bot removed the Invalid PR Title label Nov 15, 2024

stdrc marked this pull request as draft November 15, 2024 07:20

stdrc marked this pull request as ready for review November 15, 2024 07:46

graphite-app bot requested a review from a team November 15, 2024 08:08

stdrc force-pushed the rc/drop-function-dep-check branch from 162de40 to 8323c2d Compare November 15, 2024 08:32

xxchan reviewed Nov 15, 2024

View reviewed changes

e2e_test/slow_tests/udf/always_retry_python.slt Show resolved Hide resolved

xxchan approved these changes Nov 15, 2024

View reviewed changes

stdrc force-pushed the rc/drop-function-dep-check branch from 23e9181 to 42fa721 Compare November 17, 2024 17:21

BugenZhao reviewed Nov 18, 2024

View reviewed changes

stdrc force-pushed the rc/drop-function-dep-check branch from c0592b3 to 5a4b40b Compare November 19, 2024 06:04

yezizp2012 approved these changes Nov 19, 2024

View reviewed changes

stdrc added 8 commits November 21, 2024 15:43

change Binder::included_relations() to return reference instead of …

dbf4ae4

…a clone Signed-off-by: Richard Chien <[email protected]>

record referred udfs in Binder

1c314d1

Signed-off-by: Richard Chien <[email protected]>

collect object dependencies for MV

fdda26b

Signed-off-by: Richard Chien <[email protected]>

record dependencies in Request message

2849ea8

Signed-off-by: Richard Chien <[email protected]>

fix clippy

e2e37fe

Signed-off-by: Richard Chien <[email protected]>

record dependencies upon UDFs for sinks

e6b2c78

Signed-off-by: Richard Chien <[email protected]>

fix e2e

55eece9

Signed-off-by: Richard Chien <[email protected]>

add comment

946c65d

Signed-off-by: Richard Chien <[email protected]>

stdrc and others added 5 commits November 21, 2024 15:48

add e2e for drop function

75c90f7

Signed-off-by: Richard Chien <[email protected]>

Update proto/ddl_service.proto

7687ecf

Co-authored-by: Bugen Zhao <[email protected]>

use inline error matching

eb68ef1

Signed-off-by: Richard Chien <[email protected]>

add todo

aad4532

Signed-off-by: Richard Chien <[email protected]>

deprecate Sink::dependent_relations

502aa21

Signed-off-by: Richard Chien <[email protected]>

stdrc force-pushed the rc/drop-function-dep-check branch from 5a4b40b to 502aa21 Compare November 21, 2024 12:52

stdrc enabled auto-merge November 21, 2024 12:58

stdrc added this pull request to the merge queue Nov 21, 2024

graphite-app bot requested a review from a team November 21, 2024 13:37

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 21, 2024

stdrc added this pull request to the merge queue Nov 21, 2024

Merged via the queue into main with commit 37e108c Nov 21, 2024
34 of 35 checks passed

stdrc deleted the rc/drop-function-dep-check branch November 21, 2024 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(meta): dependency check for `drop function` #19399

feat(meta): dependency check for `drop function` #19399

stdrc commented Nov 15, 2024 •

edited

Loading

stdrc commented Nov 15, 2024

xxchan left a comment

xxchan Nov 15, 2024

xxchan Nov 15, 2024

xxchan Nov 15, 2024

stdrc Nov 17, 2024

BugenZhao Nov 18, 2024

BugenZhao Nov 18, 2024

stdrc Nov 19, 2024

yezizp2012 Nov 19, 2024

yezizp2012 Nov 19, 2024 •

edited

Loading

yezizp2012 left a comment

yezizp2012 Nov 19, 2024

		// TODO: to be removed, pass all objects uniformly through `dependencies` field instead.
		pub fn dependent_relations(&self) -> Vec<u32> {

feat(meta): dependency check for drop function #19399

feat(meta): dependency check for drop function #19399

Conversation

stdrc commented Nov 15, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

stdrc commented Nov 15, 2024

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

Choose a reason for hiding this comment

yezizp2012 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

yezizp2012 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(meta): dependency check for `drop function` #19399

feat(meta): dependency check for `drop function` #19399

stdrc commented Nov 15, 2024 •

edited

Loading

yezizp2012 Nov 19, 2024 •

edited

Loading