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

Merge ColumnOption::{Null, NotNull} into a single option #986

Merged
merged 6 commits into from
Nov 8, 2022

Conversation

devgony
Copy link
Collaborator

@devgony devgony commented Nov 5, 2022

To resolve #969,

Goal

Add nullable field To ColumnDef

Remove redundant ColumnOption::{NotNull, Null}

pub struct ColumnDef {
    pub name: String,
    pub data_type: DataType,
+   pub nullable: bool,
    pub options: Vec<ColumnOption>,
}

pub enum ColumnOption {
-   Null,
-   NotNull,
    Default(Expr),
    Unique { is_primary: bool },
}

impl ColumnDef {
-   pub fn is_nullable(&self) -> bool {
-       self.options
-           .iter()
-           .any(|option| option == &ColumnOption::Null)
-   }
..

Todo

  • Add nullable field to ColumnDef
  • Remove ColumnOption::{NotNull, Null},
  • ColumnDef.is_nullable

Next

  • Modify default nullable to true
  • Should handle primary_key(NotNull) and unique only (Null) properly

@devgony devgony added the enhancement New feature or request label Nov 5, 2022
@devgony devgony self-assigned this Nov 5, 2022
@coveralls
Copy link

coveralls commented Nov 5, 2022

Pull Request Test Coverage Report for Build 3421124370

  • 77 of 77 (100.0%) changed or added relevant lines in 11 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 98.497%

Files with Coverage Reduction New Missed Lines %
core/src/executor/validate.rs 1 99.32%
Totals Coverage Status
Change from base Build 3420272719: 0.1%
Covered Lines: 39529
Relevant Lines: 40132

💛 - Coveralls

@devgony devgony force-pushed the ColumnDef-nullable branch 2 times, most recently from 619aec6 to 9e3a076 Compare November 8, 2022 01:36
@panarch panarch added improvement Improvements for existing features and removed enhancement New feature or request labels Nov 8, 2022
core/src/data/value/mod.rs Outdated Show resolved Hide resolved
core/src/executor/evaluate/evaluated.rs Outdated Show resolved Hide resolved
@@ -118,7 +117,7 @@ impl Row {
} = column_def;

value.validate_type(data_type)?;
value.validate_null(nullable)?;
value.validate_null(nullable.to_owned())?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_owned() for Copy impl value might be.. too much.
you can simply use *nullable for these cases.

@devgony devgony requested a review from panarch November 8, 2022 16:06
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also the meaningful improvement.
Looks so so good.
Thanks a lot 👍

@panarch panarch merged commit 6d147f5 into gluesql:main Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvements for existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge ColumnOption::{Null, NotNull} into a single option
3 participants