-
Notifications
You must be signed in to change notification settings - Fork 968
Fix: parsing quoted built-in data types #5810
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
26159a3
to
7de5fb9
Compare
self.raise_error("Unexpected identifier", self._prev) | ||
try: | ||
tokens = self.dialect.tokenize(identifier.name) | ||
except TokenError: |
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.
Should we self.raise_error(...)
here? Otherwise, we'll now silently continue on
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.
No, this is intentional. See previous threads & Slack.
if self.dialect.SUPPORTS_USER_DEFINED_TYPES: | ||
this = self._parse_user_defined_type(identifier) | ||
else: | ||
self._retreat(self._index - 1) | ||
return None |
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.
[Nit] Could we revert the if/elif/else chain to reduce some of the indentation?
self.validate_identity('1::"int"', "CAST(1 AS INT)") | ||
assert parse_one('1::"int"', read="postgres").to.is_type(exp.DataType.Type.INT) |
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.
We don't need the generic parse_one
here if we read into the same dialect as in the testing file.
We can grab the AST as ast = self.validate_identity(...)
or do ast = self.parse_one(...)
which will set the appropriate dialect.
self.validate_identity('1::"int"', "CAST(1 AS INTEGER)") | ||
assert parse_one('1::"int"', read="redshift").to.is_type(exp.DataType.Type.INT) |
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.
Ditto
Merging this as is, will clean up tests afterwards. |
Consider a cast to a built-in data type, where the type is quoted:
1::"integer"
We currently parse this into the correct underlying type for dialects that allow this AND support user-defined types (like tsql).
This PR allows us to parse correctly for dialects that do not support user-defined types (like redshift).