Skip to content

Conversation

KKould
Copy link
Member

@KKould KKould commented Sep 12, 2025

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

Summary

DEFAULT <expr>
| { AUTOINCREMENT | IDENTITY } [
    {
        ( <start_num>, <step_num> )
        | START <num> INCREMENT <num>
    }
] [ { ORDER | NOORDER } ]

when creating a table or adding a column, the column can be set to Auto Increment.

AUTOINCREMENT will generate a sequence bound to the column and will be deleted when the column is deleted.

create or replace table t11 (c1 int, c2 int auto increment, c3 float autoincrement);

insert into t11 (c1) values(0);
insert into t11 (c1) values(0);
insert into t11 (c1) values(0);

select * from t11;
╭───────────────────────────────────────────────────────╮
│        c1       │        c2       │         c3        │
│ Nullable(Int32) │ Nullable(Int32) │ Nullable(Float32) │
├─────────────────┼─────────────────┼───────────────────┤
│               033 │
│               022 │
│               011 │
│               000 │
╰───────────────────────────────────────────────────────╯

Tips:
currently, AutoIncrementStorageIdent is used to implement next_val_v1 for AutoIncrementIdent, but since the result of next_val_v1 is incorrect, it now defaults to using next_val_v0 for AutoIncrement, which will cause AutoIncrementStorageIdent to be dead code for the time being.

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 Sep 12, 2025
@KKould KKould requested a review from TCeason September 12, 2025 05:52
@KKould KKould self-assigned this Sep 12, 2025
@TCeason
Copy link
Collaborator

TCeason commented Sep 12, 2025

  1. If table dropped the internal seq will be dropped ?

  2. Now we support rbac about seq how to skip the privilege check?

@TCeason
Copy link
Collaborator

TCeason commented Sep 12, 2025

In main branch Sequence already support to support and start, step.

And now sequence in Databend only support order.

@KKould
Copy link
Member Author

KKould commented Sep 12, 2025

  1. If table dropped the internal seq will be dropped ?
  2. Now we support rbac about seq how to skip the privilege check?
  1. The sequence will be deleted when the bound column is deleted. This test case covers this.
  2. Maybe the privilege check for auto increment should be ignored

@wubx
Copy link
Member

wubx commented Sep 12, 2025

https://docs.snowflake.com/en/sql-reference/sql/create-table

Snowflake use autoincrement as the keyword. Maybe Databend can also follow this approach.

@KKould KKould changed the title feat: impl Keyword AUTO INCREMENT feat: impl Keyword AUTOINCREMENT Sep 13, 2025
@KKould KKould force-pushed the feat/auto_increment branch 3 times, most recently from a30f025 to b2cf7cf Compare September 16, 2025 09:26
@KKould KKould marked this pull request as ready for review September 16, 2025 10:11
@KKould KKould requested a review from drmingdrmer as a code owner September 16, 2025 10:11
@KKould KKould requested a review from TCeason September 16, 2025 10:11
@TCeason
Copy link
Collaborator

TCeason commented Sep 16, 2025

ci test_ut error

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.

@drmingdrmer reviewed 4 of 25 files at r1, 3 of 35 files at r3, 3 of 12 files at r5, all commit messages.
Reviewable status: 10 of 50 files reviewed, 12 unresolved discussions (waiting on @dantengsky, @KKould, and @TCeason)


src/query/ast/src/ast/statements/auto_increment.rs line 25 at r5 (raw file):

    pub start: u64,
    pub step: u64,
    pub is_order: bool,

this field is not that obvious about the purpose. please add doc comment to explain it.
does it mean is_ordered or something else?


src/query/ast/src/ast/statements/auto_increment.rs line 43 at r5 (raw file):

        }
        Ok(())
    }

is this Display used to print out a runnable SQL? if it is, use another function or implement another trait for such purpose. Display may not provide any grammar correctness guarantee and further refactoring may break it.

Code quote:

impl Display for AutoIncrement {
    fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
        write!(f, "AUTOINCREMENT ({}, {})", self.start, self.step)?;
        if self.is_order {
            write!(f, " ORDER")?;
        } else {
            write!(f, " NOORDER")?;
        }
        Ok(())
    }

src/query/sql/src/planner/binder/default_expr.rs line 57 at r5 (raw file):

/// Helper for binding scalar expression with `BindContext`.
pub struct DefaultExprBinder {
    auto_increment_table_id: Option<MetaId>,

what does this field mean? add doc comment to explain please

@KKould
Copy link
Member Author

KKould commented Sep 17, 2025

@drmingdrmer the current review comment has been completed, and the auto increment sequence is also completed in the same transaction when the table is created.
but auto increment sequence is difficult to run the add column in the same transaction, and the current implementation also complies with "add the sequence before adding the column", so the add column has not been changed.

@KKould KKould requested a review from drmingdrmer September 17, 2025 08:33
@KKould KKould force-pushed the feat/auto_increment branch from d427567 to b1b37b1 Compare September 17, 2025 10:34
@drmingdrmer drmingdrmer requested a review from TCeason September 25, 2025 08:03
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.

@drmingdrmer reviewed 9 of 58 files at r22, 4 of 14 files at r24, 6 of 11 files at r25, all commit messages.
Reviewable status: 24 of 98 files reviewed, 29 unresolved discussions (waiting on @dantengsky, @KKould, and @TCeason)


src/meta/app/src/principal/auto_increment.rs line 37 at r25 (raw file):

impl Display for AutoIncrementKey {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}/{}", self.table_id, self.column_id)

it would be much better to be with text AutoIncrement, for example, AutoIncrement({table_id}/{column_id})


src/meta/app/src/schema/auto_increment.rs line 25 at r25 (raw file):

    pub expr: AutoIncrementExpr,
    pub key: AutoIncrementKey,
    pub count: u64,

add doc comment to struct and field. explain the purposes. In this case, the purpose of expr is not very obvious.

Code quote:

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct GetAutoIncrementNextValueReq {
    pub tenant: Tenant,
    pub expr: AutoIncrementExpr,
    pub key: AutoIncrementKey,
    pub count: u64,

src/meta/api/src/table_api.rs line 465 at r25 (raw file):

                    txn.if_then
                        .extend(vec![txn_put_pb(&storage_ident, &storage_value)?]);
                }

There is a question. Suppose there is a table with two versions, v1 and v2, in the meta - service. In v1, there is an auto - increment column with id = 3. In v2, there are two auto - increment columns with id = 3 and id = 4. At this time, if we perform garbage collection (GC) on v1, we cannot delete the auto - increment with id = 3. At this moment, we must compare the two versions before and after to check whether the later version uses the auto - increment in the current version. Where is this mechanism processed?

Code quote:

                        .push(txn_op_put(&key_dbid_tbname, serialize_u64(table_id)?))
                }

                for table_field in req.table_meta.schema.fields() {
                    let Some(auto_increment_expr) = table_field.auto_increment_expr() else {
                        continue;
                    };

                    let auto_increment_key =
                        AutoIncrementKey::new(table_id, table_field.column_id());
                    let storage_ident =
                        AutoIncrementStorageIdent::new_generic(req.tenant(), auto_increment_key);
                    let storage_value =
                        Id::new_typed(AutoIncrementStorageValue(auto_increment_expr.start));
                    txn.if_then
                        .extend(vec![txn_put_pb(&storage_ident, &storage_value)?]);
                }

src/meta/protos/proto/metadata.proto line 51 at r25 (raw file):

  uint64 min_reader_ver = 101;

  uint64 start = 1;

start should be stored in the auto increment storage, does it need to be stored here?


src/meta/protos/proto/metadata.proto line 54 at r25 (raw file):

  int64 step = 2;
  bool is_ordered = 3;
  uint32 column_id = 4;

why does it need to store column_id?


src/meta/api/src/schema_api_test_suite.rs line 5043 at r25 (raw file):

                step: 1,
                is_ordered: true,
            }))]))

the new AutoIncrementApi should be tested too.

@KKould KKould force-pushed the feat/auto_increment branch from 0b7f6a6 to e4ccfb6 Compare September 25, 2025 08:38
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.

@drmingdrmer reviewed 1 of 10 files at r12, 10 of 58 files at r22, 1 of 23 files at r23, 15 of 17 files at r26, all commit messages.
Reviewable status: 41 of 101 files reviewed, 26 unresolved discussions (waiting on @dantengsky, @KKould, and @TCeason)


src/query/service/src/catalogs/default/database_catalog.rs line 892 at r26 (raw file):

        req: GetAutoIncrementNextValueReq,
    ) -> Result<GetAutoIncrementNextValueReply> {
        self.mutable_catalog.get_autoincrement_next_value(req).await

Why does auto-increment need to be accessed via CatalogAPI? AFAIK, it only needs to be used when writing data.

Code quote:

    async fn get_autoincrement_next_value(
        &self,
        req: GetAutoIncrementNextValueReq,
    ) -> Result<GetAutoIncrementNextValueReply> {
        self.mutable_catalog.get_autoincrement_next_value(req).await

@KKould
Copy link
Member Author

KKould commented Sep 25, 2025

Why does auto-increment need to be accessed via CatalogAPI? AFAIK, it only needs to be used when writing data.

@drmingdrmer When inserting data, if a column is defined as AUTOINCREMENT and no explicit value is provided, the DefaultExprBinder generates an AsyncFunction. This function is then evaluated in TransformAsyncFunction, where the catalog is used to perform the necessary metadata operations. This constitutes the current end-to-end process for filling unspecified values during insertion.

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.

@drmingdrmer reviewed 2 of 58 files at r22, 2 of 23 files at r23.
Reviewable status: 45 of 101 files reviewed, 26 unresolved discussions (waiting on @dantengsky, @KKould, and @TCeason)

@TCeason
Copy link
Collaborator

TCeason commented Sep 26, 2025

CI error

@KKould KKould force-pushed the feat/auto_increment branch from ffa7c0c to 90a70ff Compare September 26, 2025 09:40
@KKould KKould requested a review from b41sh September 26, 2025 10:00
@KKould KKould force-pushed the feat/auto_increment branch from 815550f to 90b5b97 Compare September 26, 2025 11:29
@drmingdrmer drmingdrmer requested a review from TCeason September 26, 2025 13:10
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.

@drmingdrmer reviewed 1 of 10 files at r12, 1 of 58 files at r22, 2 of 14 files at r24, 1 of 11 files at r25, 1 of 15 files at r28, 8 of 11 files at r29, all commit messages.
Reviewable status: 51 of 102 files reviewed, 27 unresolved discussions (waiting on @dantengsky, @KKould, and @TCeason)

@KKould KKould merged commit 79d0986 into databendlabs:main Sep 26, 2025
168 of 171 checks passed
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.

7 participants