feat(query): Procedure support drop if exists and create or replace

TCeason commented 1 month ago

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

[x] support drop procedure if exists
[x] support create or replace procedure

Part of https://github.com/datafuselabs/databend/issues/16501

Tests

[x] Unit Test
[x] Logic Test
[ ] Benchmark Test
[ ] No Test - Explain why

Type of change

[ ] Bug Fix (non-breaking change which fixes an issue)
[x] New Feature (non-breaking change which adds functionality)
[ ] Breaking Change (fix or feature that could cause existing functionality not to work as expected)
[ ] Documentation Update
[ ] Refactoring
[ ] Performance Improvement
[ ] Other (please describe):

This change is

BohuTANG commented 1 month ago

Also need the syntax CREATE [ OR REPLACE ] PROCEDURE <name> ...

TCeason commented 1 month ago

Also need the syntax CREATE [ OR REPLACE ] PROCEDURE <name> ...

Yes create or replace already in plan. https://github.com/datafuselabs/databend/issues/16501

TCeason commented 1 month ago

Reviewed 5 of 7 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages. Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @sundy-li and @TCeason)

_src/query/management/src/procedure/procedure_mgr.rs line 94 at r2 (raw file):_
                    } else {
                        Err(AppError::from(name_ident.unknown_error(func_name!())).into())
                    }
IMHO, CreateOrReplace does not expect an unknown error. If there is no such record, it should just create one.

The above create_id_value() should be already enough. I do not get why this update_id_value is requried.

Code quote:
                    let res = self
                        .kv_api
                        .update_id_value(name_ident, meta.clone())
                        .await?;

                    if let Some((id, _meta)) = res {
                        Ok(CreateProcedureReply { procedure_id: *id })
                    } else {
                        Err(AppError::from(name_ident.unknown_error(func_name!())).into())
                    }
_src/query/management/src/procedure/procedure_mgr.rs line 120 at r2 (raw file):_
                return Err(AppError::from(name_ident.unknown_error("drop procedure")).into());
            }
        }
The if_exists flag only affects the response to the client; it does not influence operations on the meta-service.

This if_exists check should be moved up the call stack to the point where an Error response is actually required, either in the immediate caller or a higher-level caller.

Adn DropProcedureReq does not need if_exists at all.

Code quote:
        if dropped.is_none() {
            if req.if_exists {
                // Ok
            } else {
                return Err(AppError::from(name_ident.unknown_error("drop procedure")).into());
            }
        }

Ok, we can refactor these two function. e.g. add update_procedure API. If is replace , call update.

TCeason commented 1 month ago

The logic looks good to me. And I'd suggest avoiding using ErrorCode if possible.

Reviewed 11 of 11 files at r8, all commit messages. _Reviewable_ status: all files reviewed, 5 unresolved discussions (waiting on @sundy-li and @TCeason)

_src/query/service/src/interpreters/interpreter_procedure_create.rs line 71 at r8 (raw file):_
                return Err(e);
            }
        }
I'd suggest avoid using ErrorCode if possible. ErrorCode includes all kind of errors that makes it difficult to distinguish(by enumerate every of them) RPC error or logic error like PROCEDURE_ALREADY_EXISTS.

If add_procedure() returns KVAppError or Result<Result<..>>, the error handling like this would be easier.

Such as with Result<Result<T, LogicError>, MetaError>, a simple ? will filter out meta service errors that are not interested.

Code quote:
        if let Err(e) = UserApiProvider::instance()
            .add_procedure(&tenant, create_procedure_req, overriding)
            .await
        {
            if !(self.plan.create_option == CreateOption::CreateIfNotExists
                && e.code() == ErrorCode::PROCEDURE_ALREADY_EXISTS)
            {
                return Err(e);
            }
        }

Like this: https://github.com/datafuselabs/databend/pull/16500/commits/72137817193a33bbee4e23b32b441ae2e2486a9d?

Maybe we can refactor all procedure API in next pr that support alter procedure name?

drmingdrmer commented 1 month ago

Like this: 7213781?

Maybe we can refactor all procedure API in next pr that support alter procedure name?

Yes, It would be alright with another PR:)

In the code snippet above, the outer Result should be a std::result::Result with MetaError as its error type. The current ErrorCode Result combines both logical errors and RPC errors, which can lead to confusion for the caller. Separating these error types will provide clearer error handling and improve the API's usability.

TCeason commented 1 month ago

Also need the syntax CREATE [ OR REPLACE ] PROCEDURE <name> ...

I think we can merge this pr now. cc @BohuTANG

databendlabs / databend