Support casting to/from text for datums and float64s, respectively#1086
Support casting to/from text for datums and float64s, respectively#1086quodlibetor merged 2 commits intoMaterializeInc:masterfrom quodlibetor:support-text-casts
Conversation
This is how it works in postgres
| } | ||
|
|
||
| pub fn cast_datum_to_string<'a>(a: Datum<'a>) -> Datum<'a> { | ||
| let owned = |v: String| Datum::String(Cow::Owned(v)); |
There was a problem hiding this comment.
This is nice!
I had added cow_from_str to the Datum impl in a previous PR. Would it make sense to add this there too?
There was a problem hiding this comment.
I think that conceptually this is doing a slightly slightly different, although the fact that it matches on every kind of datum makes it look like it belongs in there.
| Datum::Bytes(_) => unreachable!("bytes should be caught in the cast match"), | ||
| Datum::String(v) => Datum::String(v), | ||
| } | ||
| } |
There was a problem hiding this comment.
As a general rule there shouldn't be any type matching in func.rs, since the correct function can be selected ahead of time by the SQL planner. We've slipped on this rule in a few places, so fine to merge as is and I'll come through and clean it up later! (But if you're up for it, great!)
There was a problem hiding this comment.
If I'm reading your comment correctly then originally I had something more like what you're requesting: I had a separate Cast___ToString method variants and equivalent methods for everything, but since each seemed to be identical I thought this would get us every cast in one fell swoop.
I see what you mean about this violating the pattern, though. I'm pretty happy to clean this up in a follow up commit, what are the benefits to this pattern? Is it mostly the code hygiene, or are there other things as well?
There was a problem hiding this comment.
I guess it means that we don't need to have a branch in the main code?
There was a problem hiding this comment.
well there'd still be the branch, but it'd by highly predictable.
src/expr/scalar/func.rs
Outdated
| UnaryFunc::CastDateToTimestamp => f.write_str("datetots"), | ||
| UnaryFunc::CastDateToTimestampTz => f.write_str("datetotstz"), | ||
| UnaryFunc::CastTimestampToTimestampTz => f.write_str("tstotstz"), | ||
| UnaryFunc::CastDatumToString => f.write_str("f64tostring"), |
There was a problem hiding this comment.
s/f64tostring/datumtostring/
|
Yep, exactly. (And one day there may not be a branch, if we move to just
transmuting things.)
…On Tue, Nov 26, 2019 at 1:28 PM Brandon W Maister ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/expr/scalar/func.rs
<#1086 (comment)>
:
> + Datum::True => Datum::cow_from_str("true"),
+ Datum::Int32(v) => owned(v.to_string()),
+ Datum::Int64(v) => owned(v.to_string()),
+ Datum::Float32(v) => owned(v.to_string()),
+ Datum::Float64(v) => owned(v.to_string()),
+ Datum::Date(v) => owned(v.to_string()),
+ Datum::Timestamp(v) => owned(v.to_string()),
+ Datum::TimestampTz(v) => owned(v.to_string()),
+ Datum::Interval(v) => owned(v.to_string()),
+ // TODO: pip the precision and scale from the scalartype to here
+ Datum::Decimal(_) => unreachable!("dec should be caught in the cast match"),
+ // TODO: not sure what postgres does here
+ Datum::Bytes(_) => unreachable!("bytes should be caught in the cast match"),
+ Datum::String(v) => Datum::String(v),
+ }
+}
well there'd still be the branch, but it'd by highly predictable.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1086?email_source=notifications&email_token=AAGXSIGUVKC5EL7BN5IVVSTQVVTDHA5CNFSM4JRPEXO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNBZDKQ#discussion_r350910144>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGXSIAM6MW7VB74NODNTBDQVVTDHANCNFSM4JRPEXOQ>
.
|
This punts on decimals and bytes because of mild inconvenience and not knowing what the output should be, respectively.
|
Okay, I'll improve that in a follow up, I'd like to get my grafana changes landed first. |
3 participants
No description provided.