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

Fix parsing CLOB, BINARY, VARBINARY, BLOB data type #618

Merged
merged 2 commits into from Sep 28, 2022

Conversation

ding-young
Copy link
Contributor

@ding-young ding-young commented Sep 21, 2022

Related Issue

Resolves #518

Description

Parse error occurs for CLOB, BINARY, VARBINARY, and BLOB data types though they are included as variants for data type & keywords in sqlparser. Since all these 4 data types contain (non-optional) u64 value, added parse_precision fn.

@coveralls
Copy link

coveralls commented Sep 26, 2022

Pull Request Test Coverage Report for Build 3141114749

  • 33 of 35 (94.29%) changed or added relevant lines in 2 files are covered.
  • 730 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.008%) to 85.58%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 5 7 71.43%
Files with Coverage Reduction New Missed Lines %
src/test_utils.rs 1 83.58%
tests/sqlparser_postgres.rs 5 97.72%
src/ast/value.rs 8 88.06%
src/tokenizer.rs 46 89.42%
tests/sqlparser_common.rs 83 97.07%
src/ast/mod.rs 267 77.22%
src/parser.rs 320 83.02%
Totals Coverage Status
Change from base Build 3090626203: -0.008%
Covered Lines: 10143
Relevant Lines: 11852

💛 - Coveralls

Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you for this @ding-young -- the code looks good. Can you please add regression tests so that this syntax is not accidentally broken in the future?

Thank you

Comment on lines +1651 to +1659
let sql = "SELECT CAST(id AS CLOB(50)) FROM customer";
let select = verified_only_select(sql);
assert_eq!(
&Expr::Cast {
expr: Box::new(Expr::Identifier(Ident::new("id"))),
data_type: DataType::Clob(50)
},
expr_from_projection(only(&select.projection))
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alamb I added test cases for parse_cast with these data types since I could not find test fn parse_datatype.. Thanks for review.

Copy link
Collaborator

@alamb alamb 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 @ding-young

@alamb alamb merged commit 6c8f31c into sqlparser-rs:main Sep 28, 2022
@AugustoFKL
Copy link
Contributor

@alamb @ding-young

CREATE TABLE tb (id BINARY); works on MySQL, so the precision is actually optional

The same for CREATE TABLE tb_3 (id BLOB);

This seems a regression of support for me.

@AugustoFKL
Copy link
Contributor

image

As the standard shows, BINARY and BLOB have optional precision

@alamb
Copy link
Collaborator

alamb commented Sep 30, 2022

Thanks for the report @AugustoFKL -- any chance you can make a PR to fix it?

@AugustoFKL
Copy link
Contributor

AugustoFKL commented Sep 30, 2022

@alamb sure, I can do it today

I also need to add precision to TIMESTAMP, TIMESTAMPZ, DATETIME, and TIME, can you wait for those before a new release, please?

I don't like to do multiple things in one PR, so I'll possibly open two (or one for each one?)

@alamb
Copy link
Collaborator

alamb commented Sep 30, 2022

Multiple small PRs would be most preferable. 🙏

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

Successfully merging this pull request may close these issues.

VARBINARY or BLOB columns not parsed
4 participants