Skip to content
This repository has been archived by the owner on Sep 28, 2021. It is now read-only.

Support serial data types. #135

Merged

Conversation

suhaibaffan
Copy link
Contributor

@suhaibaffan suhaibaffan commented Jul 3, 2020

Closes #96
Please give your input @alex-dukhno!

@suhaibaffan suhaibaffan force-pushed the Support-serial-data-types-#96 branch 2 times, most recently from d2e2ef5 to 4b277b8 Compare July 3, 2020 06:22
Copy link
Owner

@alex-dukhno alex-dukhno left a comment

Choose a reason for hiding this comment

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

Thanks for PR. I think instead of introducing new enum variants we can extend existing SmallInt, Integer and BigInt. So you should have like:

SqlType::SmallInt(min),
SqlType::Integer(min),
SqlType::BigInt(min),

and also extend respective *TypeConstraint and you need to update sql_engine so that when user creates table with smallint type then we create SqlType::SmallInt(u16::MIN_VALUE) but if with smallserialthen SqlType::SmallInt(1).

it is actually what PostgreSQL does.

postgres=# create table serials(col1 smallserial);
postgres=# \d serials
                              Table "public.serials"
 Column |   Type   | Collation | Nullable |                Default
--------+----------+-----------+----------+---------------------------------------
 col1   | smallint |           | not null | nextval('serials_col1_seq'::regclass)

@suhaibaffan
Copy link
Contributor Author

Got it, thanks @alex-dukhno.

@suhaibaffan
Copy link
Contributor Author

@alex-dukhno let me know if I am on the right direction or not?

@suhaibaffan suhaibaffan force-pushed the Support-serial-data-types-#96 branch 3 times, most recently from d0eff35 to 40959d7 Compare July 6, 2020 02:55
Copy link
Owner

@alex-dukhno alex-dukhno left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only minor comments. To fix rustfmt complaints you can run cargo +nightly fmt

Ok(_) => Ok(()),
Err(e) if e.code == lexical::ErrorCode::InvalidDigit => Err(ConstraintError::NotAnInt),
Err(_) => Err(ConstraintError::OutOfRange),
if self.min < lexical::parse::<u16, _>(in_value).unwrap() {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please move if check inside Ok(parsed) branch, unwraping will panic and in_value will be parsed only once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks.

Ok(_) => Ok(()),
Err(e) if e.code == lexical::ErrorCode::InvalidDigit => Err(ConstraintError::NotAnInt),
Err(_) => Err(ConstraintError::OutOfRange),
if self.min < lexical::parse::<u32, _>(in_value).unwrap() {
Copy link
Owner

Choose a reason for hiding this comment

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

same as above

Err(e) if e.code == lexical::ErrorCode::InvalidDigit => Err(ConstraintError::NotAnInt),
Err(_) => Err(ConstraintError::OutOfRange),
if self.min < lexical::parse::<u64, _>(in_value).unwrap() {
match lexical::parse::<u64, _>(in_value) {
Copy link
Owner

Choose a reason for hiding this comment

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

same as above

@suhaibaffan suhaibaffan force-pushed the Support-serial-data-types-#96 branch from 19aa9ed to 49be91c Compare July 6, 2020 08:44
@alex-dukhno
Copy link
Owner

You can check clippy errors with ./ci/clippy-local.sh
cargo test --lib --all - unit tests and
pytest -v tests/functional/* - functional end-to-end tests

@suhaibaffan
Copy link
Contributor Author

@alex-dukhno In sql_engine/src/ddl/create_table,
sqlparser::ast::DataType::Regclass => SqlType::Integer(1), I can use Regclass for just Serial, what do you suggest for smallSerial and bigSerial?

@alex-dukhno
Copy link
Owner

I am not sure about Regclass ... I bet that Custom(ObjectName) is what you are looking for https://github.com/ballista-compute/sqlparser-rs/blob/main/src/ast/data_type.rs#L67
But I can be wrong 😄

@suhaibaffan suhaibaffan force-pushed the Support-serial-data-types-#96 branch from 783e1b8 to 841ab2e Compare July 7, 2020 10:41
@suhaibaffan suhaibaffan force-pushed the Support-serial-data-types-#96 branch from 2e70ca0 to 1a24746 Compare July 8, 2020 15:31
@suhaibaffan suhaibaffan force-pushed the Support-serial-data-types-#96 branch from 1a24746 to c9fea1d Compare July 8, 2020 15:35
sqlparser::ast::DataType::Char(len) => SqlType::Char(len.unwrap_or(255)),
sqlparser::ast::DataType::Varchar(len) => SqlType::VarChar(len.unwrap_or(255)),
sqlparser::ast::DataType::Boolean => SqlType::Bool,
sqlparser::ast::DataType::Custom(ObjectName(_serial)) => SqlType::Integer(1),
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
sqlparser::ast::DataType::Custom(ObjectName(_serial)) => SqlType::Integer(1),
sqlparser::ast::DataType::Custom(name) => {
let name = name.to_string();
if name == "smallserial" {
SqlType::SmallInt(1);
} else if name == "serial" {
SqlType::Integer(1);
} else if name == "bigserial" {
SqlType::BigInt(1);
} else {
unimplemented!("{:?} type is not supported", name)
}
},

Copy link
Owner

Choose a reason for hiding this comment

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

this should help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alex-dukhno fixed, please review it.

@suhaibaffan suhaibaffan force-pushed the Support-serial-data-types-#96 branch from d1927b6 to 0323093 Compare July 8, 2020 17:08
@alex-dukhno
Copy link
Owner

I'll merge it as I am currently working on type improvements

@alex-dukhno alex-dukhno merged commit 78bfd26 into alex-dukhno:master Jul 8, 2020
@alex-dukhno
Copy link
Owner

Thanks for the PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support serial data types
2 participants