fix(query): fix table stats incorrect when create table with exists snapshot

[TCeason]

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

Summary

fix table stats incorrect when create table with exists snapshot

When create table with options SNAPSHOT_LOCATION,

after create table , will judge whether the SNAPSHOT_LOCATION actually exists.

• If exists, will regenerate the table statistic and recreate table.

• if not exists, will drop the table and return err.

Closes #10406

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
databend	⬜️ Ignored (Inspect)			Mar 8, 2023 at 11:32AM (UTC)

[dantengsky]

https://github.com/datafuselabs/databend/blob/84ef1b7bd63310a6b905f2e4448b59efb52c6b34/src/query/service/src/interpreters/interpreter_table_create.rs#L193-L200

            let snapshot = snapshot_reader.read(&params).await;
            match snapshot {
                Ok(snapshot) => {
                     .... 
                }
                Err(e) => { 
                    catalog
                        .drop_table_by_id(DropTableByIdReq {
                            if_exists: true,
                            tb_id: table.get_table_info().ident.table_id,
                        })
                        .await?;
                    return Err(e);
                }
            }
        }

will a new table be left there with incorrect statistics , if catalog.drop_table_by_id failed, let's say, caused by a network error?

---

How about this?

• with snapshot_loc, and MetaReaders::table_snapshot_reader
  we can read the TableSnapshot
  the version of the table snapshot can be extracted by using this
  https://github.com/datafuselabs/databend/blob/f9badf477bbcb7ff86c490e63dfc743659614bda/src/query/storages/fuse/src/io/locations.rs#L110
• from the TableSnapshot.summary we can build the TableStatictics
  https://github.com/datafuselabs/databend/blob/77379b5b2240d76ff83cc91247fbf4bd07baba3f/src/query/storages/common/table-meta/src/meta/v2/snapshot.rs#L52

[dantengsky]

How about this?
....

O... the operator is not there, how about using the ctx.get_data_operator

https://github.com/datafuselabs/databend/blob/829e236b12bac80157588919feef63938c0b4ce9/src/query/catalog/src/table_context.rs#L125

technically, it is not correct, but

• it seems to be able to meet the "current requirement"
• cloning a table by using magic option is more like a tmp workaround

[TCeason]

https://github.com/datafuselabs/databend/blob/84ef1b7bd63310a6b905f2e4448b59efb52c6b34/src/query/service/src/interpreters/interpreter_table_create.rs#L193-L200

            let snapshot = snapshot_reader.read(&params).await;
            match snapshot {
                Ok(snapshot) => {
                     .... 
                }
                Err(e) => { 
                    catalog
                        .drop_table_by_id(DropTableByIdReq {
                            if_exists: true,
                            tb_id: table.get_table_info().ident.table_id,
                        })
                        .await?;
                    return Err(e);
                }
            }
        }

will a new table be left there with incorrect statistics , if catalog.drop_table_by_id failed, let's say, caused by a network error?
How about this?

• with snapshot_loc, and MetaReaders::table_snapshot_reader
  we can read the TableSnapshot
  the version of the table snapshot can be extracted by using this
  https://github.com/datafuselabs/databend/blob/f9badf477bbcb7ff86c490e63dfc743659614bda/src/query/storages/fuse/src/io/locations.rs#L110
• from the TableSnapshot.summary we can build the TableStatictics
  https://github.com/datafuselabs/databend/blob/77379b5b2240d76ff83cc91247fbf4bd07baba3f/src/query/storages/common/table-meta/src/meta/v2/snapshot.rs#L52

O... the operator

We can do this modify

    async fn table_statistics(&self) -> Result<Option<TableStatistics>> {
        let s = &self.table_info.meta.statistics;
        if s.number_of_rows != 0 {
            Ok(Some(TableStatistics {
                num_rows: Some(s.number_of_rows),
                data_size: Some(s.data_bytes),
                data_size_compressed: Some(s.compressed_data_bytes),
                index_size: Some(s.index_data_bytes),
            }))
        } else {
            // add read snapshot in there
        }
    }

If num_rows is 0 means the new table or not stat is incorrect, we can read snapshot.

[dantengsky]

If num_rows is 0 means the new table or not stat is incorrect, we can read snapshot.

not sure if num_rows is 0 indicates that the stat is incorrect. (seems we should not conclude like this)

besides: there is an implicit constraint for the Table::table_statistics,
this method is used in management mode , which can not access object storage .

[dantengsky]

If so , how about triggering a backoff: #10424 (comment)

not sure about this, @sundy-li we need your help

[TCeason]

If so , how about triggering a backoff: #10424 (comment)

not sure about this, @sundy-li we need your help

We can not do this. Because the root cause is stats incorrect, in there we can backoff ,but join or other use stats will also generate err result.

[sundy-li]

The current implementation is enough to fix the issue without too much effort.

[TCeason]

The current implementation is enough to fix the issue without too much effort.

A hack usage generates a hack modify pr.. How about supporting https://docs.snowflake.com/en/sql-reference/sql/create-clone in a later version?

[sundy-li]

The current implementation is enough to fix the issue without too much effort.

A hack usage generates a hack modify pr.. How about supporting docs.snowflake.com/en/sql-reference/sql/create-clone in a later version?

That could be great.

set hide_options_in_show_create_table = 0; is hard to use.