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

support bigger blob #314

Merged
merged 2 commits into from
May 28, 2022
Merged

support bigger blob #314

merged 2 commits into from
May 28, 2022

Conversation

hunjixin
Copy link
Contributor

PR Info

support medium blob longblob type

Adds

Breaking Changes

Changes

this implement is a little track , but in the current implementation there is specific blob type

@hunjixin hunjixin force-pushed the support/bigger_blob branch from 7cb4b0d to 8d8f8b7 Compare April 27, 2022 05:48
@tyt2y3
Copy link
Member

tyt2y3 commented Apr 27, 2022

This does not seem right. We should instead add more variants to ColumnType

@billy1624
Copy link
Member

This does not seem right. We should instead add more variants to ColumnType

But then all these new types, mediumblob and longblob, are not supported in other db backend. Hence, it will be maps to blob datatype in other db. Hope it won't confuse the users.

@hunjixin hunjixin closed this May 4, 2022
@hunjixin hunjixin reopened this May 4, 2022
@hunjixin
Copy link
Contributor Author

hunjixin commented May 4, 2022

This does not seem right. We should instead add more variants to ColumnType

But then all these new types, mediumblob and longblob, are not supported in other db backend. Hence, it will be maps to blob datatype in other db. Hope it won't confuse the users.

best way to support blob is add new column type.
It may not be practical to use a set of type system to completely unify the types of several databases.
consider anyway to enable the database to support unique types

@billy1624
Copy link
Member

consider anyway to enable the database to support unique types

Are you suggesting ColumnType that are feature guarded by db backend? I don't think this is a good idea. We try to avoid relying on feature guard by db backend.

But then all these new types, mediumblob and longblob, are not supported in other db backend. Hence, it will be maps to blob datatype in other db.

This will be a good middle ground for me though we have to be explicit these new column types are primarily added for MySQL.

Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

I think a blob type with a enum BlobSize param makes the most sense

@billy1624
Copy link
Member

I think a blob type with a enum BlobSize param makes the most sense

You mean blob type with an Option<BlobSize> param?

@tyt2y3
Copy link
Member

tyt2y3 commented May 4, 2022

Okay this might not actually be relevant.

My rule of thumb is "anything that exists conceptually deserves to be named accordingly".

@tyt2y3
Copy link
Member

tyt2y3 commented May 12, 2022

It could be

enum BlobSize {
Default,
Medium,
Long,
}

so we don't need an Option

@billy1624
Copy link
Member

Hey @hunjixin, got any updates? :)

@hunjixin
Copy link
Contributor Author

Hey @hunjixin, got any updates? :)

add new columntype "BLOB" to support this feature? if all think it okay, i'd like to implement one, but i am not sure whether affect other db types, maybe need help.

@billy1624
Copy link
Member

My plans are...

  1. Add BlobSize to Binary variant in ColumnType enum
    Binary(BlobSize)
    With BlobSize defined as
    enum BlobSize {
        Default(Option<u32>), // Backward compatibility: MySQL & SQLite support `binary(length)` column type
        Medium,
        Long,
    }
  2. Add helper methods to ColumnDef, namely binary_medium and binary_long
  3. Update SQL builder in all three backends, if binary_medium and binary_long are not support in a particular backend then both of them will be mapped to binary

CC @tyt2y3 @hunjixin

@hunjixin
Copy link
Contributor Author

My plans are...

  1. Add BlobSize to Binary variant in ColumnType enum

    Binary(BlobSize)

    With BlobSize defined as

    enum BlobSize {
        Default(Option<u32>), // Backward compatibility: MySQL & SQLite support `binary(length)` column type
        Medium,
        Long,
    }
  2. Add helper methods to ColumnDef, namely binary_medium and binary_long

  3. Update SQL builder in all three backends, if binary_medium and binary_long are not support in a particular backend then both of them will be mapped to binary

CC @tyt2y3 @hunjixin

see your idea, but have two suggestions

  1. its better to include blob/tinyblob include blobsize?
#[derive(Debug, Clone)]
enum BlobSize {
    Default(Option<u32>), // Backward compatibility: MySQL & SQLite support `binary(length)` column type
    Tiny,
    Blob,  //default blob, need a better name?
    Medium,
    Long,
}
  1. maybe more appropriate to add the blob method, so as not to be confused with binary
    /// Set column type as blob
    pub fn blob(&mut self, size: BlobSize) -> &mut Self {
        //todo need to check set default size or convert to correct blob type?
        self.types = Some(ColumnType::Binary(size));
        self
    }

@hunjixin hunjixin changed the base branch from master to 0.14.x May 21, 2022 04:01
@hunjixin hunjixin changed the base branch from 0.14.x to master May 21, 2022 04:01
@hunjixin hunjixin force-pushed the support/bigger_blob branch from 8d8f8b7 to be5f186 Compare May 21, 2022 04:04
@hunjixin
Copy link
Contributor Author

@billy1624 updated pr

src/table/column.rs Outdated Show resolved Hide resolved
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks! @hunjixin

@billy1624
Copy link
Member

@tyt2y3 tyt2y3 merged commit 679e879 into SeaQL:master May 28, 2022
@tyt2y3
Copy link
Member

tyt2y3 commented May 28, 2022

Thank you for the patience @hunjixin

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
Development

Successfully merging this pull request may close these issues.

3 participants