-
Notifications
You must be signed in to change notification settings - Fork 605
Distinguish between CHAR VARYING
, CHARACTER VARYING
, and CHARACTER
data types, differentiating them from VARCHAR
and CHAR
#648
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
Conversation
@alamb ready for review :D |
Pull Request Test Coverage Report for Build 3178311617
💛 - Coveralls |
Thank you @AugustoFKL |
This PR appears to have a conflict -- can you please resolve that @AugustoFKL ? |
character string types
CHAR VARYING
, CHARACTER VARYING
, and CHARACTER
data types, differentiating them from VARCHAR
and CHAR
@alamb done :) About the coverage, I'll add tests as I expand the ANSII data types. Once I finish all of them (I plan to do that this week), I'll add tests to cover additional data types that aren't on ANSII (e.g., DOUBLE) |
@@ -5288,80 +5295,46 @@ mod tests { | |||
}); | |||
} | |||
|
|||
// TODO add tests for all data types? https://github.com/sqlparser-rs/sqlparser-rs/issues/2 | |||
// TODO when we have dialect validation by data type parsing, split test | |||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we have lost coverage after the rebase (e.g. the new tests for TIME WITHOUT TIMEZONE
for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb that's expected, as I mentioned in my comment above.
I'll add tests along the week, trying to reach 100% coverage for data type parsing. But for now, I'd like to restrict this to the ones I added if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this PR would remove coverage for types that were added in other PRs. I am fine with expanding coverage in future PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. It was because I (dumbly) did the structure for tests twice, as it didn't please me. I'll re-add the coverage for those data types, and for now leave as "additional data types" for those that aren't covered by ANSII
@alamb done |
…entiating between each one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AugustoFKL
Adding support for
CHAR VARYING
,CHARACTER VARYING
, andCHARACTER
data types, differentiating them fromVARCHAR
andCHAR
, which were mistakenly considered the same (1).The precision is optional to support PostgreSQL's syntax (2).
Also, adding tests to start resolving #2, but that's a WIP.
[1] : https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#character-string-type
[2] : https://www.postgresql.org/docs/current/datatype-character.html
Resolves: #647