-
-
Notifications
You must be signed in to change notification settings - Fork 539
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
Rewrite enum and text casting #973
Conversation
src/query/helper.rs
Outdated
fn cast_enum_text_inner<C, F>(expr: Expr, col: &C, f: F) -> SimpleExpr | ||
where | ||
C: ColumnTrait, | ||
F: Fn(Expr, &String) -> SimpleExpr, |
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.
Same here, I might have lost the context, but why are we passing enum name around as String? Can we pass around DynIden instead?
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.
The String
comes from ColumnType::Enum
Line 81 in 87f7891
Enum(String, Vec<String>), |
I rewrite it into F: Fn(Expr, DynIden) -> SimpleExpr
, but still, we need to wrap the String
as DynIden
like SeaRc::new(Alias::new(enum_name))
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.
Um... I think we should have used an Iden here. Second question is, how often is this ColumnType
object being cloned? If it is being cloned frequently then we'd better Arc
it. At the very least I think a named struct serves as a better documentation:
EnumDef {
name: DynIden,
variants: Vec<DynIden>,
}
* Rewrite enum and text casting * Add doc tests * Refactoring
PR Info
fn column()
also handle enum type #960Adds
QuerySelect::column
method also handle enum typeChanges