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

Supports rename table #660

Closed
bonito-skipjack opened this issue Nov 29, 2022 · 12 comments
Closed

Supports rename table #660

bonito-skipjack opened this issue Nov 29, 2022 · 12 comments
Labels
C-feature Category Features

Comments

@bonito-skipjack
Copy link

What problem does the new feature solve?

Can not change the table name

What does the feature do?

Rename old_table to new_table

Implementation challenges

No response

@bonito-skipjack bonito-skipjack added the C-feature Category Features label Nov 29, 2022
@evenyag
Copy link
Contributor

evenyag commented Nov 29, 2022

Thanks, RENAME TABLE is really a nice feature!

To implement the RENAME TABLE statement, the most challenging part is to deal with the table's storage path. Now we use the table name as part of the path. We might need to store the actual path in the system catalog or consider only using table id in the path. @v0y4g3r @killme2008

@e1ijah1
Copy link
Contributor

e1ijah1 commented Dec 7, 2022

Thanks, RENAME TABLE is really a nice feature!

To implement the RENAME TABLE statement, the most challenging part is to deal with the table's storage path. Now we use the table name as part of the path. We might need to store the actual path in the system catalog or consider only using table id in the path. @v0y4g3r @killme2008

Hi, I'd like to work on this issue. Should we get the table name from the manifest file if we only use the table_id to create the path?

@killme2008
Copy link
Contributor

Thanks, RENAME TABLE is really a nice feature!
To implement the RENAME TABLE statement, the most challenging part is to deal with the table's storage path. Now we use the table name as part of the path. We might need to store the actual path in the system catalog or consider only using table id in the path. @v0y4g3r @killme2008

Hi, I'd like to work on this issue. Should we get the table name from the manifest file if we only use the table_id to create the path?

Yes. The table metadata is already stored in the manifest and recovered when opening the table. I think only using table id as the table data path is acceptable and easier to support RENAME TABLE. Feel free to try, thank you.

@evenyag
Copy link
Contributor

evenyag commented Dec 8, 2022

Now we use {schema_name}/{table_name}_{table_id} as data path

fn table_dir(schema_name: &str, table_name: &str, table_id: TableId) -> String {
format!("{}/{}_{}/", schema_name, table_name, table_id)
}

I think we could change it to {schema_name}/{table_id}. We might need to introduce a schema id if we'd like to support RENAME SCHEMA.

I'd suggest creating a tracking issue for this feature, as we might need to

  • change the table path to schema/table_id
  • support renaming tables in the mito table engine, this may require to
    • persist the change to manifest (maybe a new MetaAction is required)
    • change the in-memory metadata
    • recover the change when reopening the table
  • support renaming tables in the catalog manager
    • update the catalog's metadata
    • support this both in standalone and cluster mode
  • support parsing the RENAME TABLE statements in the parser
  • execute the rename statement

Changing the table path and implementing rename table for the table engine should be easier for beginners.

@killme2008
Copy link
Contributor

killme2008 commented Dec 8, 2022

  • support renaming tables in the mito table engine, this may require to

    • persist the change to manifest (maybe a new MetaAction is required)
    • change the in-memory metadata
    • recover the change when reopening the table
  • support renaming tables in the catalog manager

No need to create a new MetaAction, use RegionChange is enough for this feature. And the recovery logic is already there.

@e1ijah1
Copy link
Contributor

e1ijah1 commented Dec 8, 2022

  • support renaming tables in the mito table engine, this may require to

    • persist the change to manifest (maybe a new MetaAction is required)
    • change the in-memory metadata
    • recover the change when reopening the table
  • support renaming tables in the catalog manager

No need to create a new MetaAction, use RegionChange is enough for this feature. And the recovery logic is already there.

Since the RegionChange does not contain information about the table name, and as the documentation states, "Mito table engine divides a table into one or multiple regions," maybe we should maintain the table name change by the MetaAction.

@killme2008
Copy link
Contributor

killme2008 commented Dec 8, 2022

Since the RegionChange does not contain information about the table name, and as the documentation states, "Mito table engine divides a table into one or multiple regions," maybe we should maintain the table name change by the MetaAction.

@e1ijah1
Sorry, I meant TableChange indeed, my mistake.

pub struct TableChange {
    pub table_info: RawTableInfo,
}

The RawTableInfo already contains table_name:

pub struct RawTableInfo {
    ....
    pub name: String,
    ....
}

I think we can create a new TableInfo from the old one when renaming the table, update its name and persist the change to manifest, then set the new metadata into MitoTable.

@killme2008
Copy link
Contributor

killme2008 commented Dec 8, 2022

@e1ijah1
Persists the change to manifest, you can refer to https://github.com/GreptimeTeam/greptimedb/blob/develop/src/mito/src/table.rs#L362-L374

And you should update the table_info in MitoTable after persistence, it's the in-memory table metadata.

@e1ijah1
Copy link
Contributor

e1ijah1 commented Dec 11, 2022

Now we use {schema_name}/{table_name}_{table_id} as data path

fn table_dir(schema_name: &str, table_name: &str, table_id: TableId) -> String {
format!("{}/{}_{}/", schema_name, table_name, table_id)
}

I think we could change it to {schema_name}/{table_id}. We might need to introduce a schema id if we'd like to support RENAME SCHEMA.

I'd suggest creating a tracking issue for this feature, as we might need to

  • change the table path to schema/table_id

  • support renaming tables in the mito table engine, this may require to

    • persist the change to manifest (maybe a new MetaAction is required)
    • change the in-memory metadata
    • recover the change when reopening the table
  • support renaming tables in the catalog manager

    • update the catalog's metadata
    • support this both in standalone and cluster mode
  • support parsing the RENAME TABLE statements in the parser

  • execute the rename statement

Changing the table path and implementing rename table for the table engine should be easier for beginners.

It looks like that when registering a table, a record is written to the system_catalog table, but the Table trait does not provide a way to modify or delete a record. Perhaps the implementation of registering a table needs to be restructured to decouple the specific table name from the entry in the system catalog.

pub async fn register_table(
&self,
catalog: String,
schema: String,
table_name: String,
table_id: TableId,
) -> crate::error::Result<usize> {
let full_table_name = format_full_table_name(&catalog, &schema, &table_name);
let request = build_table_insert_request(full_table_name, table_id);
self.information_schema
.system
.insert(request)
.await
.context(InsertCatalogRecordSnafu)
}

async fn insert(&self, request: InsertRequest) -> table::error::Result<usize> {
self.table.insert(request).await
}

@evenyag
Copy link
Contributor

evenyag commented Dec 13, 2022

@e1ijah1 Sorry for the late reply.

but the Table trait does not provide a way to modify or delete a record

To modify a record, you could just put an entry with the same key, which overwrites the old one. But deletion requires #614, I will work on it after finishing #555.

Changing the table path and implementing rename table for the table engine should be easier for beginners.
So I'd suggest implementing rename in the mito table engine first, then the catalog manager, in a bottom-up manner, which doesn't require interaction with the catalog or the system catalog table. Then once the deletion feature is done, we could start working on the rest parts.

Another important thing is that we use table name as part of the entry key, so we might need to use the table id instead.

pub fn build_table_insert_request(full_table_name: String, table_id: TableId) -> InsertRequest {
build_insert_request(
EntryType::Table,
full_table_name.as_bytes(),
serde_json::to_string(&TableEntryValue { table_id })
.unwrap()
.as_bytes(),
)
}

@e1ijah1
Copy link
Contributor

e1ijah1 commented Dec 13, 2022

@e1ijah1 Sorry for the late reply.

but the Table trait does not provide a way to modify or delete a record

To modify a record, you could just put an entry with the same key, which overwrites the old one. But deletion requires #614, I will work on it after finishing #555.

Changing the table path and implementing rename table for the table engine should be easier for beginners.
So I'd suggest implementing rename in the mito table engine first, then the catalog manager, in a bottom-up manner, which doesn't require interaction with the catalog or the system catalog table. Then once the deletion feature is done, we could start working on the rest parts.

Another important thing is that we use table name as part of the entry key, so we might need to use the table id instead.

pub fn build_table_insert_request(full_table_name: String, table_id: TableId) -> InsertRequest {
build_insert_request(
EntryType::Table,
full_table_name.as_bytes(),
serde_json::to_string(&TableEntryValue { table_id })
.unwrap()
.as_bytes(),
)
}

got it & thanks

@killme2008
Copy link
Contributor

Already implemented. Tracked in #723.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category Features
Projects
None yet
Development

No branches or pull requests

4 participants