feat(query): support create iceberg table with partition and properties #17812

TCeason · 2025-04-18T11:29:24Z

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

Summary

CREATE TABLE [IF NOT EXISTS] [database.]table_name
(column_definition1[, column_definition2, ...])
[PARTITION BY (par_col1[, par_col2...])]
[PROPERTIES ("key" = "value", ...)]

Note:

partition key supports data types other than FLOAT, DOUBLE, DECIMAL, and DATETIME, and does not support NULL values.
If create table that in default/hive catalog assign partition or PROPERTIES it will be failed
Add new struct TablePartition. Only support identity transform.
forbidan alter column name when table engine is iceberg
Table PROPERTIES and PARTITION will send to iceberg-rust by CreateTableReq

fixes: Feature: create iceberg table with partition and properties #17810

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

TCeason · 2025-04-19T00:08:18Z

Rerun ci, wait #17813 merge.

tests/sqllogictests/suites/tpch_iceberg/utils.test

src/query/service/src/interpreters/interpreter_table_create.rs

drmingdrmer

Reviewed 2 of 47 files at r1, 20 of 49 files at r3.
Reviewable status: 22 of 57 files reviewed, 4 unresolved discussions (waiting on @BohuTANG, @sundy-li, @TCeason, and @Xuanwo)

src/meta/app/src/schema/table.rs line 257 at r3 (raw file):

#[derive(serde::Serialize, serde::Deserialize, Clone, Debug, Eq, PartialEq)]
pub enum TablePartition {
    Identity { columns: Vec<String> },

Is it safe to use column name as partition keys, while user might rename a column.

TCeason · 2025-04-20T01:39:45Z

Reviewed 2 of 47 files at r1, 20 of 49 files at r3.
Reviewable status: 22 of 57 files reviewed, 4 unresolved discussions (waiting on @BohuTANG, @sundy-li, @TCeason, and @Xuanwo)

src/meta/app/src/schema/table.rs line 257 at r3 (raw file):
#[derive(serde::Serialize, serde::Deserialize, Clone, Debug, Eq, PartialEq)]
pub enum TablePartition {
    Identity { columns: Vec<String> },
Is it safe to use column name as partition keys, while user might rename a column.

A good issue.

I think we should forbidden iceberg table rename col name and return a definite error
. Like view and stream engine

https://github.com/databendlabs/databend/blob/main/src/query/service/src/interpreters/interpreter_table_rename_column.rs#L76

Xuanwo · 2025-04-20T14:56:48Z

Hi, I’m a bit concerned that we’re planning to store this data in metasrv. Are we intending to use metasrv as an iceberg catalog, or will we support creating iceberg tables as internal tables, similar to what the Fuse engine does?

TCeason · 2025-04-20T15:01:41Z

Hi, I’m a bit concerned that we’re planning to store this data in metasrv. Are we intending to use metasrv as an iceberg catalog, or will we support creating iceberg tables as internal tables, similar to what the Fuse engine does?

Actually we can add the partition info in req not tablemeta.

If we do not store this information, we must load table when we use it later

Xuanwo · 2025-04-20T15:04:29Z

Actually we can add the partition info in req not tablemeta.

If we do not store this information, we must load table when we use it later

It depends on our design and our objectives. Typically, we can't assume that we are the only ones writing to or reading from an iceberg table. Users can perform various actions on it, such as renaming column names with Spark. While we can cache this metadata, we can't persist it.

TCeason · 2025-04-20T15:09:40Z

Actually we can add the partition info in req not tablemeta.

If we do not store this information, we must load table when we use it later

It depends on our design and our objectives. Typically, we can't assume that we are the only ones writing to or reading from an iceberg table. Users can perform various actions on it, such as renaming column names with Spark. While we can cache this metadata, we can't persist it.

If so it's better to add these info in req. maybe we need to cost some time and discuss it tomorrow

drmingdrmer

Reviewed 1 of 47 files at r1, 1 of 38 files at r6, all commit messages.
Reviewable status: 2 of 66 files reviewed, 4 unresolved discussions (waiting on @BohuTANG, @sundy-li, @TCeason, and @Xuanwo)

src/meta/app/src/schema/table.rs line 257 at r3 (raw file):

Previously, TCeason wrote…

A good issue.

I think we should forbidden iceberg table rename col name and return a definite error
. Like view and stream engine

https://github.com/databendlabs/databend/blob/main/src/query/service/src/interpreters/interpreter_table_rename_column.rs#L76

Then how to deal with the column rename? 🤔

TCeason · 2025-04-21T07:50:32Z

4. PROPERTIES and PARTITION

For write : We set the partition in Req not store these info.
For read: We load the data in iceberg catalog

drmingdrmer

Reviewed 3 of 47 files at r1, 1 of 25 files at r2, 23 of 52 files at r5, 37 of 38 files at r6.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @BohuTANG, @sundy-li, @TCeason, and @Xuanwo)

src/query/storages/iceberg/src/database.rs line 201 at r6 (raw file):

        };

        let table_create_option = if let Some(ref partition) = req.table_partition {

The deep nesting since this line makes the logic hard to understand. It would be better extract these part into another method and flatten the nesting logic.

BTW, the builder should be created only once.

src/query/service/src/interpreters/interpreter_table_create.rs line 497 at r6 (raw file):

                    columns: self.plan.table_partition.clone(),
                })
            },

Use uniformed type for property and partition.

All types should be defined with Option, or none types with Option.

Code quote:

            table_properties: if self.plan.table_properties.is_empty() {
                None
            } else {
                Some(self.plan.table_properties.clone())
            },
            table_partition: if self.plan.table_partition.is_empty() {
                None
            } else {
                Some(TablePartition::Identity {
                    columns: self.plan.table_partition.clone(),
                })
            },

src/query/sql/src/planner/binder/ddl/table.rs line 489 at r6 (raw file):

                table_partition.push(iceberg_table_partition.to_string());
            }
        }

iceberg_table_partition.into_iter().collect() would be good enough to convert a BTreeMap to Vec?

Code quote:

        let mut table_partition: Vec<String> = Vec::new();
        if let Some(iceberg_table_partition) = iceberg_table_partition {
            for iceberg_table_partition in iceberg_table_partition {
                table_partition.push(iceberg_table_partition.to_string());
            }
        }

src/meta/app/src/schema/table.rs line 441 at r6 (raw file):

        TablePartition::Identity { columns: vec![] }
    }
}

Why not derive Default for it?

Code quote:

impl Default for TablePartition {
    fn default() -> Self {
        TablePartition::Identity { columns: vec![] }
    }
}

src/query/sql/src/planner/plans/ddl/table.rs line 56 at r6 (raw file):

    pub options: TableOptions,
    pub table_properties: TableOptions,
    pub table_partition: Vec<String>,

If partitioning is optional, these two field should be optional.

If the default value, such as vec![] indicating no-partitioning, everywhere else should use vec![] to represent a no-partitioning decision.

To me, Option<Vec<String>> would be more accurate about the purpose.

Code quote:

    pub table_properties: TableOptions,
    pub table_partition: Vec<String>,

TCeason

Dismissed @drmingdrmer from a discussion.
Reviewable status: 50 of 66 files reviewed, 7 unresolved discussions (waiting on @BohuTANG, @drmingdrmer, @sundy-li, and @Xuanwo)

src/meta/app/src/schema/table.rs line 441 at r6 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Why not derive Default for it?

Maybe support other type in the future. So I impl a Default.

src/query/storages/iceberg/src/database.rs line 201 at r6 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

The deep nesting since this line makes the logic hard to understand. It would be better extract these part into another method and flatten the nesting logic.

BTW, the builder should be created only once.

Done

TCeason

Reviewable status: 50 of 66 files reviewed, 7 unresolved discussions (waiting on @BohuTANG, @drmingdrmer, @sundy-li, and @Xuanwo)

src/query/sql/src/planner/binder/ddl/table.rs line 489 at r6 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

iceberg_table_partition.into_iter().collect() would be good enough to convert a BTreeMap to Vec?

Done.

…ERTIES it will be failed

2. iceberg_table_properties rename to table_properties 3. iceberg_partition rename to table_partition

…eTableReq

TCeason · 2025-04-22T06:19:24Z

All conversations were resolved, ci passed. Please review this pr. cc @Xuanwo @drmingdrmer @sundy-li

TCeason requested a review from drmingdrmer as a code owner April 18, 2025 11:29

TCeason marked this pull request as draft April 18, 2025 11:29

github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Apr 18, 2025

TCeason force-pushed the iceberg_table branch 2 times, most recently from eaa5937 to e40546e Compare April 18, 2025 13:26

TCeason marked this pull request as ready for review April 18, 2025 20:44

TCeason requested review from Xuanwo and sundy-li April 18, 2025 20:45

TCeason force-pushed the iceberg_table branch from e40546e to 93f9da4 Compare April 18, 2025 21:18

BohuTANG reviewed Apr 19, 2025

View reviewed changes

tests/sqllogictests/suites/tpch_iceberg/utils.test Show resolved Hide resolved

TCeason marked this pull request as draft April 19, 2025 01:47

TCeason force-pushed the iceberg_table branch 3 times, most recently from e142d45 to 1c22e47 Compare April 19, 2025 14:38

TCeason marked this pull request as ready for review April 19, 2025 23:14

TCeason requested a review from BohuTANG April 19, 2025 23:14

sundy-li reviewed Apr 19, 2025

View reviewed changes

src/query/service/src/interpreters/interpreter_table_create.rs Outdated Show resolved Hide resolved

sundy-li reviewed Apr 19, 2025

View reviewed changes

src/query/service/src/interpreters/interpreter_table_create.rs Outdated Show resolved Hide resolved

TCeason force-pushed the iceberg_table branch 2 times, most recently from ba0cf39 to dd6cfe0 Compare April 20, 2025 00:23

TCeason requested a review from sundy-li April 20, 2025 00:24

TCeason force-pushed the iceberg_table branch from dd6cfe0 to a99a604 Compare April 20, 2025 01:11

drmingdrmer requested changes Apr 20, 2025

View reviewed changes

TCeason force-pushed the iceberg_table branch from a99a604 to b7a68f5 Compare April 20, 2025 07:33

TCeason force-pushed the iceberg_table branch 2 times, most recently from ab84fe5 to a4dcf71 Compare April 21, 2025 07:29

TCeason requested a review from drmingdrmer April 21, 2025 07:29

drmingdrmer reviewed Apr 21, 2025

View reviewed changes

drmingdrmer requested changes Apr 21, 2025

View reviewed changes

TCeason commented Apr 21, 2025

View reviewed changes

TCeason force-pushed the iceberg_table branch 4 times, most recently from add45f4 to 4534df3 Compare April 22, 2025 02:49

TCeason added 8 commits April 22, 2025 11:59

feat(query): support create iceberg table with partition and properties

d7a7a77

If create table that in default/hive catalog assign partition or PROP…

51125be

…ERTIES it will be failed

1. trait Catalog add new function support_partition

8c41b4d

2. iceberg_table_properties rename to table_properties 3. iceberg_partition rename to table_partition

iceberg table do not support alter column name

a617757

refactor: table_partition/properties refactor from TableMeta to Creat…

aed8d64

…eTableReq

fix ci

5d49017

fix conversation and optimize code

6eccbc5

refactor support_partition

367d1bd

TCeason mentioned this pull request Apr 22, 2025

chore(ci): add test_stateless_standalone timeout minutes to 18 #17828

Merged

11 tasks

TCeason force-pushed the iceberg_table branch from 4534df3 to 367d1bd Compare April 22, 2025 04:03

TCeason requested a review from drmingdrmer April 22, 2025 06:19

sundy-li approved these changes Apr 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(query): support create iceberg table with partition and properties #17812

feat(query): support create iceberg table with partition and properties #17812

TCeason commented Apr 18, 2025 •

edited

Loading

TCeason commented Apr 19, 2025

drmingdrmer left a comment

TCeason commented Apr 20, 2025 •

edited

Loading

Xuanwo commented Apr 20, 2025 •

edited

Loading

TCeason commented Apr 20, 2025

Xuanwo commented Apr 20, 2025

TCeason commented Apr 20, 2025

drmingdrmer left a comment

TCeason commented Apr 21, 2025

drmingdrmer left a comment

TCeason left a comment

TCeason left a comment

TCeason commented Apr 22, 2025

feat(query): support create iceberg table with partition and properties #17812

Are you sure you want to change the base?

feat(query): support create iceberg table with partition and properties #17812

Conversation

TCeason commented Apr 18, 2025 • edited Loading

Summary

Tests

Type of change

TCeason commented Apr 19, 2025

drmingdrmer left a comment

Choose a reason for hiding this comment

TCeason commented Apr 20, 2025 • edited Loading

Xuanwo commented Apr 20, 2025 • edited Loading

TCeason commented Apr 20, 2025

Xuanwo commented Apr 20, 2025

TCeason commented Apr 20, 2025

drmingdrmer left a comment

Choose a reason for hiding this comment

TCeason commented Apr 21, 2025

drmingdrmer left a comment

Choose a reason for hiding this comment

TCeason left a comment

Choose a reason for hiding this comment

TCeason left a comment

Choose a reason for hiding this comment

TCeason commented Apr 22, 2025

TCeason commented Apr 18, 2025 •

edited

Loading

TCeason commented Apr 20, 2025 •

edited

Loading

Xuanwo commented Apr 20, 2025 •

edited

Loading