variant_schema udf#37
Conversation
|
@friendlymatthew, you can take a look |
friendlymatthew
left a comment
friendlymatthew
left a comment
There was a problem hiding this comment.
Thanks @sdf-jk . Directionally this makes sense but I think it needs a simplification pass
I think there's a bunch of leftover infrastructure from the removed UDAF that has no callers in the scalar UDF, we can probably strip this out.
The type widening logic is also incomplete and makes me a bit nervous. Plus it only kicks in when merging elements within a single array. I'd rather just do equality checks for now and add widening in a follow up with full support
| /// Prints schema in a presentable manner | ||
| pub fn print_schema(schema: &VariantSchema) -> String { | ||
| match schema { | ||
| VariantSchema::Primitive(s) => format!("{s}"), | ||
|
|
||
| VariantSchema::Variant => "VARIANT".to_string(), | ||
|
|
||
| VariantSchema::Array(inner) => { | ||
| format!("ARRAY<{}>", print_schema(inner)) | ||
| } | ||
|
|
||
| VariantSchema::Object(fields) => { | ||
| let parts: Vec<String> = fields | ||
| .iter() | ||
| .map(|(k, v)| format!("{k}: {}", print_schema(v))) | ||
| .collect(); | ||
| format!("OBJECT<{}>", parts.join(", ")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
seems we are reinventing std::fmt::Display.
We could probably just impl Display and call sites use variant_schema.to_string() or format!("{variant_schema"})
There was a problem hiding this comment.
I tried that earlier and it didn't look good. I can give it another shot, just to show you how it looks.
I misread that. This sounds good.
friendlymatthew
left a comment
friendlymatthew
left a comment
There was a problem hiding this comment.
Thanks @sdf-jk . Directionally this makes sense but I think it needs a simplification pass
I think there's a bunch of leftover infrastructure from the removed UDAF that has no callers in the scalar UDF, we can probably strip this out.
The type widening logic is also incomplete and makes me a bit nervous. Plus it only kicks in when merging elements within a single array. I'd rather just do equality checks for now and add widening in a follow up with full support
|
Sorry I think I removed serde_json when I was performing a manual merge |
|
@friendlymatthew thanks, I'll address the comments.
No worries |
|
@friendlymatthew thanks, I've addressed your comments. Ready for another pass. |
2 participants
variant_schemainitial support #28