-
Notifications
You must be signed in to change notification settings - Fork 174
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
feat: Implement to_string for subtype #980
Conversation
Hey @andygrove @parthchandra @viirya let me know if this is good enough or something has to be improved |
Apologies @adi-kmt I had not seen this PR. I will start reviewing today. |
There is a build error:
|
@@ -662,6 +662,10 @@ impl PhysicalPlanner { | |||
let child = self.create_expr(expr.child.as_ref().unwrap(), input_schema)?; | |||
Ok(Arc::new(ToJson::new(child, &expr.timezone))) | |||
} | |||
ExprStruct::ToString(expr) => { | |||
let child = self.create_expr(expr.child.as_ref().unwrap(), input_schema)?; | |||
Ok(Arc::new(To_String::new(child, &expr.timezone))) |
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.
Could you change the name to ToString
without the underscore?
case StructsToString(options, child, timezoneId) => | ||
if (options.nonEmpty) { |
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.
It looks like the formatting is off here. You can run make format
to fix this,
Hi @adi-kmt. I just spent some time looking through this PR and I think there is some confusion that I can help with. The issue #814 is for implementing CAST from struct to string. In SQL, this would look like
|
This is now implemented in #1066 |
Which issue does this PR close?
Closes #814
Rationale for this change
What changes are included in this PR?
Adding the To string cast
How are these changes tested?
Rust tests are written and tested.