-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Parse Postgres citext as Type::Unknown #94
Conversation
src/postgres/parser/column.rs
Outdated
pub fn parse_enum_variants(mut self, enums: &HashMap<String, Vec<String>>) -> Self { | ||
let mut enum_name = None; | ||
let mut no_enum_variants = true; | ||
if let Type::Enum(ref mut enum_def) = self.col_type { | ||
enum_name = Some(&enum_def.typename); | ||
if let Some(def) = enums.get(&enum_def.typename) { | ||
enum_def.values = def.clone() | ||
no_enum_variants = def.is_empty(); | ||
enum_def.values = def.clone(); | ||
} | ||
} | ||
// An enum column without any variants will be treated as unknown column type | ||
if let (Some(enum_name), true) = (enum_name, no_enum_variants) { | ||
self.col_type = Type::Unknown(enum_name.into()); | ||
} |
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.
Why enum
has something to do with citext
?
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.
Postgres column parser has two steps.
It will determine the data type of each column based on type name without any knowledge of database enums. All user-defined
types (enum and citext both belong to this subset) will be considered as enum.
Then, the second step will fill the enum variants for each enum column, i.e. the code snippet you highlighted.
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 get it now. It then seems to highlight a coding-style problem: the caller should first determine whether it's a enum or not, perhaps adding a method is_enum
and then dispatches to parse_enum
or parse_custom_type
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.
Alright, that make sense. I'm going to rewrite it now.
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.
@tyt2y3 please check again
src/postgres/def/types.rs
Outdated
@@ -148,8 +148,8 @@ pub enum Type { | |||
impl Type { | |||
// TODO: Support more types | |||
#[allow(clippy::should_implement_trait)] | |||
pub fn from_str(name: &str) -> Type { | |||
match name.to_lowercase().as_str() { | |||
pub fn from_str(column_type: &str, udt_name: Option<&String>, is_enum: bool) -> Type { |
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: I think this can be udt_name: Option<&str>
as well
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.
Right, let me check
PR Info
citext
column type as string #89New Features
citext