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

feat(query): Support rename dictionary ddl. #16754

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

Winnie-Hong0927
Copy link
Contributor

@Winnie-Hong0927 Winnie-Hong0927 commented Nov 2, 2024

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

Summary

We implement the logical plan and binder of rename dictionary, and we can call the interface of meta to rename dictionary.
We can execute Dictionary-related SQL statements like follows.
RENAME DICTIONARY [db0.]dict_A TO [db1.]dict_B;

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • 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 Reviewable

@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Nov 2, 2024
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 27 files at r1, all commit messages.
Reviewable status: 9 of 27 files reviewed, 6 unresolved discussions (waiting on @Winnie-Hong0927)


src/meta/app/src/app_error.rs line 902 at r1 (raw file):

#[derive(thiserror::Error, Debug, Clone, PartialEq, Eq)]
#[error("UnknownDictionary: `{dictionary_name}` while `{context}`")]
pub struct UnknownDictionary {

Define Unknown error with UnknownError: https://github.com/datafuselabs/databend/blob/af5fe4c3384b220555a282a0c2e33b564d75e43c/src/meta/app/src/tenant_key/errors.rs#L106

Refer to

https://github.com/datafuselabs/databend/blob/10d54aec1e67bdc88f0a4d5e67d0c88b579ec941/src/meta/app/src/app_error.rs#L1033

So does to the exists error.


src/meta/app/src/schema/dictionary.rs line 179 at r1 (raw file):

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct RenameDictionaryReq {
    pub if_exists: bool,

remove this field if_exists. This field does not affect catalog or meta-service behavior, it just affect what to respond to the user. Move these logic to corresponding Binder or Interpreter level.


src/meta/app/src/schema/dictionary.rs line 181 at r1 (raw file):

    pub if_exists: bool,
    pub name_ident: DictionaryNameIdent,
    pub new_db_name: String,

DictionaryNameIdent does not include a db-name, the target db should be identified by database-id.


src/meta/app/src/schema/dictionary.rs line 219 at r1 (raw file):

/// The meta-service key for storing dictionary id history ever used by a dictionary name
#[derive(Clone, Debug, Eq, PartialEq, Hash)]
pub struct DictionaryIdHistoryIdent {

If Dictionary does not need to support undrop, remove this struct.


src/meta/app/src/schema/dictionary.rs line 231 at r1 (raw file):

#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub struct DictionaryIdToName {

Why is this this identifier required? Is lookup dictionary name by id required?


src/query/service/src/interpreters/interpreter_dictionary_rename.rs line 66 at r1 (raw file):

                ),
                new_db_name: self.plan.new_database.clone(),
                new_dictionary_name: self.plan.new_dictionary.clone(),

The target should also be a DictionaryIdToName, instead of db-name plus a dictionary name.

Code quote:

                new_db_name: self.plan.new_database.clone(),
                new_dictionary_name: self.plan.new_dictionary.clone(),

@b41sh b41sh self-requested a review November 2, 2024 15:32
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 27 files at r1, 10 of 14 files at r2, all commit messages.
Reviewable status: 27 of 31 files reviewed, 6 unresolved discussions (waiting on @b41sh and @Winnie-Hong0927)


src/meta/api/src/schema_api_impl.rs line 3055 at r2 (raw file):

                &req.name_ident,
                "rename_dictionary: src (db,dictionary)",
            )?;

use get_pb().ok_or_else() would be simple to handle these errors:

            let seq_meta = self.get_pb(&tbid).await?.ok_or_else(|| {
                AppError::UnknownTableId(UnknownTableId::new(req.table_id, "create_table_index"))
            })?;

https://github.com/datafuselabs/databend/blob/f8201da838d0df1ff00e2213361bdabe128156ef/src/meta/api/src/schema_api_impl.rs#L2447

Code quote:

            let (dict_id_seq, dict_id) = get_u64_value(self, &req.name_ident).await?;
            assert_dictionary_exist(
                dict_id_seq,
                &req.name_ident,
                "rename_dictionary: src (db,dictionary)",
            )?;

src/meta/api/src/schema_api_impl.rs line 3058 at r2 (raw file):

            let new_dict_id_seq = self.get_seq(&req.new_name_ident).await?;
            dict_has_to_not_exist(new_dict_id_seq, &req.new_name_ident, "rename_dictionary")?;

This client side assertion does not guarantee there is no such record when it is renamed inside meta-service.

You should have to add a condition to the following condition to assert the target record has a zero seq.

Code quote:

            let new_dict_id_seq = self.get_seq(&req.new_name_ident).await?;
            dict_has_to_not_exist(new_dict_id_seq, &req.new_name_ident, "rename_dictionary")?;

src/meta/api/src/schema_api_impl.rs line 3511 at r2 (raw file):

    name_ident: &DictionaryNameIdent,
    _ctx: impl Display,
) -> Result<(), KVAppError> {

This function should not return KVAppError, it should just return an ExistError.


src/meta/api/src/schema_api_test_suite.rs line 7503 at r2 (raw file):

                ..DatabaseMeta::default()
            },
        };

Use Util to simplify these env setup, refer to:

let mut util1 = Util::new(mt, tenant_name, db_name_1, "", "eng");
let mut util2 = Util::new(mt, tenant_name, db_name_2, "", "eng");
info!("--- create dropped db1 and db2; db2 is non-retainable");
{
util1.create_db().await?;
util1.drop_db().await?;
util2.create_db().await?;
util2.drop_db().await?;
info!("--- update db2's drop_on");

Code quote:

        let db1_name = "db1";
        let dict1_name = "dict1";
        let db2_name = "db2";
        let dict2_name = "dict2";

        let db1_req = CreateDatabaseReq {
            create_option: CreateOption::Create,
            name_ident: DatabaseNameIdent::new(&tenant, db1_name),
            meta: DatabaseMeta {
                engine: "".to_string(),
                ..DatabaseMeta::default()
            },
        };

src/meta/app/src/schema/dictionary.rs line 180 at r2 (raw file):

pub struct RenameDictionaryReq {
    pub name_ident: DictionaryNameIdent,
    pub new_name_ident: DictionaryNameIdent,

There should not be another DictionaryNameIdent, DictionaryNameIdent includes a tenant. But it does not allow to rename to a different tenant. the new_name_ident should be just DictionaryIdentity. Otherwise the SchemaApi has to check whether the two tenants are the same.


src/meta/app/src/schema/dictionary.rs line 220 at r2 (raw file):

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct RenameDictionaryReply {}

If it is empty. Do not define this type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants