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

Table Column with Default Expression #347

Closed
billy1624 opened this issue Jun 6, 2022 · 12 comments · Fixed by #436
Closed

Table Column with Default Expression #347

billy1624 opened this issue Jun 6, 2022 · 12 comments · Fixed by #436
Assignees
Milestone

Comments

@billy1624
Copy link
Member

Motivation

  • Currently, we support column with default value but not default expression
  • Having default expression in column such as timestamp / datetime is very common, e.g. CURRENT_TIMESTAMP as the default expression

Proposed Solutions

  • ColumnSpec::Default(Value) should be changed to holding SimpleExpr, i.e. ColumnSpec::Default(SimpleExpr)
    /// All column specification keywords
    #[derive(Debug, Clone)]
    pub enum ColumnSpec {
    Null,
    NotNull,
    Default(Value),
    AutoIncrement,
    UniqueKey,
    PrimaryKey,
    Extra(String),
    }
  • Keep the old default method and introduce a new method called default_expr which take any Into<SimpleExpr>
    /// Set default value of a column
    pub fn default<T>(&mut self, value: T) -> &mut Self
    where
    T: Into<Value>,
    {
    self.spec.push(ColumnSpec::Default(value.into()));
    self
    }
@billy1624 billy1624 added the good first issue Good for newcomers label Jun 6, 2022
@billy1624 billy1624 added this to the 1.0 milestone Jun 6, 2022
@billy1624 billy1624 moved this to Triage in SeaQL Dev Tracker Jun 6, 2022
@billy1624 billy1624 removed this from the 1.0 milestone Jun 6, 2022
@billy1624 billy1624 moved this from Triage to Open for Contributions in SeaQL Dev Tracker Jun 6, 2022
@ikrivosheev
Copy link
Member

@billy1624, hello! I will do this.

@ikrivosheev ikrivosheev self-assigned this Jun 6, 2022
@billy1624
Copy link
Member Author

Thanks! @ikrivosheev

@billy1624
Copy link
Member Author

@ikrivosheev ikrivosheev assigned billy1624 and unassigned ikrivosheev Jun 14, 2022
@ikrivosheev
Copy link
Member

ikrivosheev commented Jun 21, 2022

@billy1624 what do you think about new Enum for table? We only need some variants from SimpleExpr

  1. FuncionCall
  2. Value
  3. Custom

@ikrivosheev
Copy link
Member

No, bad ideas... We need to split QueryBuilder. For unavailable operations panic.

@ikrivosheev ikrivosheev assigned ikrivosheev and unassigned billy1624 Jun 22, 2022
@billy1624
Copy link
Member Author

billy1624 commented Jun 27, 2022

Hey @ikrivosheev, sorry for the delay.

@billy1624 what do you think about new Enum for table? We only need some variants from SimpleExpr

Do you mean introducing a new Enum for the "Default Value" of a column?

/// All column specification keywords
#[derive(Debug, Clone)]
pub enum ColumnSpec {
Null,
NotNull,
Default(Value),
AutoIncrement,
UniqueKey,
PrimaryKey,
Extra(String),
}

pub enum ColumnSpec {
...
    Default(DefaultValue)
...
}

// I just makeup the name, feel free to rename it :)
pub enum DefaultValue {
    Func(Function),
    Value(Value),
    Custom(String),
}

Something like this?

@ikrivosheev
Copy link
Member

Hey @ikrivosheev, sorry for the delay.

@billy1624 what do you think about new Enum for table? We only need some variants from SimpleExpr

Do you mean introducing a new Enum for the "Default Value" of a column?

/// All column specification keywords
#[derive(Debug, Clone)]
pub enum ColumnSpec {
Null,
NotNull,
Default(Value),
AutoIncrement,
UniqueKey,
PrimaryKey,
Extra(String),
}

pub enum ColumnSpec {
...
    Default(DefaultValue)
...
}

// I just makeup the name, feel free to rename it :)
pub enum DefaultValue {
    Func(Function),
    Value(Value),
    Custom(String),
}

Something like this?

Yes

@ikrivosheev
Copy link
Member

@billy1624 @tyt2y3 what do you think about custom Enum?

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 10, 2022

  1. I think we should rename the old default to default_value and the new default is an alias of default_expr
  2. Yes I think having a dedicated DefaultColumnExpr is a good idea

@ikrivosheev
Copy link
Member

@tyt2y3 @billy1624 hello! I have made several attempts and understand that is better to use SimpleExpr instead create DefaultColumnExpr

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 16, 2022

I have no objection to either

@billy1624
Copy link
Member Author

Hello @ikrivosheev, are you working on this?

@billy1624 billy1624 self-assigned this Aug 29, 2022
@billy1624 billy1624 moved this from Open for Contributions to In Progress in SeaQL Dev Tracker Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants