Repository: databendlabs/databend (language: rust)

Issue:
bug:  `LOCATION` of `CREATE TABLE` does NOT work as expected 
**Summary**


Not sure if this is a documentation bug or a implementation bug:

when creating a table with external location, following the official doc:

https://docs.databend.com/sql/sql-commands/ddl/table/ddl-create-table-external-location#syntax

> CREATE TABLE [IF NOT EXISTS] [db.]table_name (
>     <column_name> <data_type> [NOT NULL | NULL] [{ DEFAULT <expr> }],
>     <column_name> <data_type> [NOT NULL | NULL] [{ DEFAULT <expr> }],
>     ...
> )
> LOCATION = 's3://<bucket>/[<path>]'
> CONNECTION = (
>     ENDPOINT_URL = 'https://<endpoint-URL>'
>     ACCESS_KEY_ID = '<your-access-key-ID>'
>     SECRET_ACCESS_KEY = '<your-secret-access-key>'
>     REGION = '<region-name>'
>     ENABLE_VIRTUAL_HOST_STYLE = 'true' | 'false'
> )
> |
> CONNECTION = (
>     CONNECTION_NAME = '<your-connection-name>'
> );

A table of invalid location could be created successfully, mutation and query are all work,

***But the table data is NOT stored at the specified location***.


Also: In section `Create a Table with External Location`
https://docs.databend.com/sql/sql-commands/ddl/table/ddl-create-table-external-location#create-a-table-with-external-location

A different syntax is used:

~~~
-- Create a table named `mytable` and specify the location `s3://testbucket/admin/data/` for the data storage
CREATE TABLE mytable (
  a INT
)
's3://testbucket/admin/data/'
CONNECTION = (
  ACCESS_KEY_ID = '<your_aws_key_id>',
  SECRET_ACCESS_KEY = '<your_aws_secret_key>',
  ENDPOINT_URL = 'https://s3.amazonaws.com'
);
~~~




**Reporduce:**

for example, a table of with invalid s3 location can be created (incorrectly):

~~~

mysql> select version();
+----------------------------------------------------------------------------------------+
| version()                                                                              |
+----------------------------------------------------------------------------------------+
| 8.0.26-v1.2.673-nightly-f777d195be(rust-1.85.0-nightly-2024-12-18T05:01:23.526505965Z) |
+----------------------------------------------------------------------------------------+
1 row in set (0.01 sec)
Read 1 rows, 1.00 B in 0.007 sec., 153.02 rows/sec., 153.02 B/sec.

mysql> select * from system.configs where "group" = 'storage' and name = 'type';
+---------+------+-------+-------------+
| group   | name | value | description |
+---------+------+-------+-------------+
| storage | type | fs    |             |
+---------+------+-------+-------------+
1 row in set (0.03 sec)
Read 184 rows, 12.05 KiB in 0.008 sec., 21.89 thousand rows/sec., 1.40 MiB/sec.


mysql> create table t_location (c int) LOCATION = 's3 | bucket=non-exist-bucket,root=/,endpoint=https://s3.amazonaws.com';
Query OK, 0 rows affected (0.05 sec)

mysql> insert into t_location values(1);
+-------------------------+
| number of rows inserted |
+-------------------------+
|                       1 |
+-------------------------+
1 row in set (0.21 sec)
Read 1 rows, 5.00 B in 0.144 sec., 6.96 rows/sec., 34.81 B/sec.


mysql> select * from t_location;
+------+
| c    |
+------+
|    1 |
+------+
1 row in set (0.11 sec)
Read 1 rows, 5.00 B in 0.025 sec., 40.12 rows/sec., 200.61 B/sec.

~~~


Discussion / hints:

Fix patch:
diff --git a/src/query/ast/src/ast/statements/table.rs b/src/query/ast/src/ast/statements/table.rs
index 7efc87fa6b8fa..34a89d4a83d09 100644
--- a/src/query/ast/src/ast/statements/table.rs
+++ b/src/query/ast/src/ast/statements/table.rs
@@ -745,6 +745,21 @@ impl Display for Engine {
     }
 }
 
+impl From<&str> for Engine {
+    fn from(s: &str) -> Self {
+        match s.to_lowercase().as_str() {
+            "null" => Engine::Null,
+            "memory" => Engine::Memory,
+            "fuse" => Engine::Fuse,
+            "view" => Engine::View,
+            "random" => Engine::Random,
+            "iceberg" => Engine::Iceberg,
+            "delta" => Engine::Delta,
+            _ => unreachable!("invalid engine: {}", s),
+        }
+    }
+}
+
 #[derive(Debug, Clone, PartialEq, Eq, Drive, DriveMut)]
 pub enum CompactTarget {
     Block,
diff --git a/src/query/service/src/interpreters/common/table_option_validation.rs b/src/query/service/src/interpreters/common/table_option_validation.rs
index a36a217ac8b0e..b16132635d8c0 100644
--- a/src/query/service/src/interpreters/common/table_option_validation.rs
+++ b/src/query/service/src/interpreters/common/table_option_validation.rs
@@ -17,6 +17,7 @@ use std::collections::HashSet;
 use std::sync::LazyLock;
 
 use chrono::Duration;
+use databend_common_ast::ast::Engine;
 use databend_common_exception::ErrorCode;
 use databend_common_expression::TableSchemaRef;
 use databend_common_io::constants::DEFAULT_BLOCK_MAX_ROWS;
@@ -45,7 +46,7 @@ use databend_storages_common_table_meta::table::OPT_KEY_TEMP_PREFIX;
 use log::error;
 
 /// Table option keys that can occur in 'create table statement'.
-pub static CREATE_TABLE_OPTIONS: LazyLock<HashSet<&'static str>> = LazyLock::new(|| {
+pub static CREATE_FUSE_OPTIONS: LazyLock<HashSet<&'static str>> = LazyLock::new(|| {
     let mut r = HashSet::new();
     r.insert(FUSE_OPT_KEY_ROW_PER_PAGE);
     r.insert(FUSE_OPT_KEY_BLOCK_PER_SEGMENT);
@@ -64,12 +65,32 @@ pub static CREATE_TABLE_OPTIONS: LazyLock<HashSet<&'static str>> = LazyLock::new
 
     r.insert(OPT_KEY_ENGINE);
 
+    r.insert(OPT_KEY_CONNECTION_NAME);
+
+    r.insert("transient");
+    r.insert(OPT_KEY_TEMP_PREFIX);
+    r
+});
+
+pub static CREATE_LAKE_OPTIONS: LazyLock<HashSet<&'static str>> = LazyLock::new(|| {
+    let mut r = HashSet::new();
+    r.insert(OPT_KEY_ENGINE);
     r.insert(OPT_KEY_LOCATION);
     r.insert(OPT_KEY_CONNECTION_NAME);
+    r
+});
 
+pub static CREATE_RANDOM_OPTIONS: LazyLock<HashSet<&'static str>> = LazyLock::new(|| {
+    let mut r = HashSet::new();
+    r.insert(OPT_KEY_ENGINE);
     r.insert(OPT_KEY_RANDOM_SEED);
+    r
+});
 
-    r.insert("transient");
+pub static CREATE_MEMORY_OPTIONS: LazyLock<HashSet<&'static str>> = LazyLock::new(|| {
+    let mut r = HashSet::new();
+    r.insert(OPT_KEY_ENGINE);
+    r.insert(OPT_KEY_DATABASE_ID);
     r.insert(OPT_KEY_TEMP_PREFIX);
     r
 });
@@ -85,8 +106,16 @@ pub static UNSET_TABLE_OPTIONS_WHITE_LIST: LazyLock<HashSet<&'static str>> = Laz
     r
 });
 
-pub fn is_valid_create_opt<S: AsRef<str>>(opt_key: S) -> bool {
-    CREATE_TABLE_OPTIONS.contains(opt_key.as_ref().to_lowercase().as_str())
+pub fn is_valid_create_opt<S: AsRef<str>>(opt_key: S, engine: &Engine) -> bool {
+    let opt_key = opt_key.as_ref().to_lowercase();
+    let opt_key = opt_key.as_str();
+    match engine {
+        Engine::Fuse => CREATE_FUSE_OPTIONS.contains(opt_key),
+        Engine::Iceberg | Engine::Delta => CREATE_LAKE_OPTIONS.contains(&opt_key),
+        Engine::Random => CREATE_RANDOM_OPTIONS.contains(&opt_key),
+        Engine::Memory => CREATE_MEMORY_OPTIONS.contains(&opt_key),
+        Engine::Null | Engine::View => opt_key == OPT_KEY_ENGINE,
+    }
 }
 
 pub fn is_valid_block_per_segment(
diff --git a/src/query/service/src/interpreters/interpreter_table_create.rs b/src/query/service/src/interpreters/interpreter_table_create.rs
index f15937f892ccd..173d891b5d0cc 100644
--- a/src/query/service/src/interpreters/interpreter_table_create.rs
+++ b/src/query/service/src/interpreters/interpreter_table_create.rs
@@ -402,7 +402,7 @@ impl CreateTableInterpreter {
 
         for table_option in table_meta.options.iter() {
             let key = table_option.0.to_lowercase();
-            if !is_valid_create_opt(&key) {
+            if !is_valid_create_opt(&key, &self.plan.engine) {
                 error!("invalid opt for fuse table in create table statement");
                 return Err(ErrorCode::TableOptionInvalid(format!(
                     "table option {key} is invalid for create table statement",
diff --git a/src/query/service/src/interpreters/interpreter_table_set_options.rs b/src/query/service/src/interpreters/interpreter_table_set_options.rs
index 9aa7e26449fc4..2147a607ae162 100644
--- a/src/query/service/src/interpreters/interpreter_table_set_options.rs
+++ b/src/query/service/src/interpreters/interpreter_table_set_options.rs
@@ -15,6 +15,7 @@
 use std::collections::HashMap;
 use std::sync::Arc;
 
+use databend_common_ast::ast::Engine;
 use databend_common_catalog::table::TableExt;
 use databend_common_exception::ErrorCode;
 use databend_common_exception::Result;
@@ -101,9 +102,17 @@ impl Interpreter for SetOptionsInterpreter {
                 OPT_KEY_CLUSTER_TYPE
             )));
         }
+        let catalog = self.ctx.get_catalog(self.plan.catalog.as_str()).await?;
+        let database = self.plan.database.as_str();
+        let table_name = self.plan.table.as_str();
+        let table = catalog
+            .get_table(&self.ctx.get_tenant(), database, table_name)
+            .await?;
+
         for table_option in self.plan.set_options.iter() {
             let key = table_option.0.to_lowercase();
-            if !is_valid_create_opt(&key) {
+            let engine = Engine::from(table.engine());
+            if !is_valid_create_opt(&key, &engine) {
                 error!("{}", &error_str);
                 return Err(ErrorCode::TableOptionInvalid(format!(
                     "table option {key} is invalid for alter table statement",
@@ -111,12 +120,6 @@ impl Interpreter for SetOptionsInterpreter {
             }
             options_map.insert(key, Some(table_option.1.clone()));
         }
-        let catalog = self.ctx.get_catalog(self.plan.catalog.as_str()).await?;
-        let database = self.plan.database.as_str();
-        let table_name = self.plan.table.as_str();
-        let table = catalog
-            .get_table(&self.ctx.get_tenant(), database, table_name)
-            .await?;
 
         let table_version = table.get_table_info().ident.seq;
         if let Some(value) = self.plan.set_options.get(OPT_KEY_CHANGE_TRACKING) {
diff --git a/src/query/service/src/interpreters/interpreter_table_show_create.rs b/src/query/service/src/interpreters/interpreter_table_show_create.rs
index 8021e1d6f2ac9..1b69d3ef4f815 100644
--- a/src/query/service/src/interpreters/interpreter_table_show_create.rs
+++ b/src/query/service/src/interpreters/interpreter_table_show_create.rs
@@ -281,7 +281,7 @@ impl ShowCreateTableInterpreter {
 
         if engine != "ICEBERG" && engine != "DELTA" {
             if let Some(sp) = &table_info.meta.storage_params {
-                table_create_sql.push_str(format!(" LOCATION = '{}'", sp).as_str());
+                table_create_sql.push_str(format!(" '{}' ", sp).as_str());
             }
         }
 


Test patch:
diff --git a/tests/sqllogictests/suites/base/05_ddl/05_0000_ddl_create_tables.test b/tests/sqllogictests/suites/base/05_ddl/05_0000_ddl_create_tables.test
index 01e23ba86fae3..fa0b044115b69 100644
--- a/tests/sqllogictests/suites/base/05_ddl/05_0000_ddl_create_tables.test
+++ b/tests/sqllogictests/suites/base/05_ddl/05_0000_ddl_create_tables.test
@@ -303,6 +303,15 @@ create table t(a int) x=x
 statement error 1301
 create table t(a int) external_location='xxx'
 
+statement error 1301
+create table t(a int) location='xxx'
+
+statement error 1301
+create table t(a int) seed='123'
+
+statement ok
+create table t_random(a int) engine=RANDOM seed='123'
+
 statement error 1301
 create table t(a int) snapshot_loc='xxx'
 
diff --git a/tests/sqllogictests/suites/base/06_show/06_0001_show_create_table.test b/tests/sqllogictests/suites/base/06_show/06_0001_show_create_table.test
index 2020b3db6d849..558d5f33b593b 100644
--- a/tests/sqllogictests/suites/base/06_show/06_0001_show_create_table.test
+++ b/tests/sqllogictests/suites/base/06_show/06_0001_show_create_table.test
@@ -53,7 +53,7 @@ CREATE TABLE test.d (a int not null) ENGINE=FUSE 'fs:///tmp/load/files/' CONNECT
 query TT
 SHOW CREATE TABLE `test`.`d`
 ----
-d CREATE TABLE d ( a INT NOT NULL ) ENGINE=FUSE CLUSTER BY linear(a, a % 3) CLUSTER_TYPE='linear' COMPRESSION='lz4' STORAGE_FORMAT='parquet' LOCATION = 'fs | root=/tmp/load/files/'
+d CREATE TABLE d ( a INT NOT NULL ) ENGINE=FUSE CLUSTER BY linear(a, a % 3) CLUSTER_TYPE='linear' COMPRESSION='lz4' STORAGE_FORMAT='parquet' 'fs | root=/tmp/load/files/'
 
 query TT
 SHOW CREATE TABLE `test`.`c`