Postgres date-time types #2890
Postgres date-time types #2890
Conversation
|
related: #2870 |
|
Happy @itowlson to see you take this on. Edit: The client stuff was tedious. I was getting close to figuring out, but your legendary skills are the goal for me. Not there yet. |
itowlson
force-pushed
the
db-date-time-2
branch
from
b025536
to
594892e
Compare
itowlson
force-pushed
the
db-date-time-2
branch
from
594892e
to
7314253
Compare
| .ok_or_else(|| anyhow!("invalid date y={y}, m={mon}, d={d}"))?; | ||
| Ok(Box::new(naive_date)) | ||
| } | ||
| ParameterValue::Time((h, min, s, ns)) => { |
There was a problem hiding this comment.
Would we support time zone input? It looks like Postgres allows some standard timezone extensions that a user could provide as an optional parameter.
Alternatively, we can document in the WIT that UTC is assumed.
There was a problem hiding this comment.
Timezones is a good question and I am not sure of the answer. Postgres seemed to have two timestamp types, one with and one without timezones. @tyler-harpool as the first person to pick this up and the person with the strongest interest in Postgres date types, how do you feel about this?
| str(string), | ||
| binary(list<u8>), | ||
| date(tuple<s32, u8, u8>), // (year, month, day) | ||
| time(tuple<u8, u8, u8, u32>), // (hour, minute, second, nanosecond) |
There was a problem hiding this comment.
Did we consider letting users provide strings for date and time?
There was a problem hiding this comment.
Strings at the WIT level? No, these specifications follow @tyler-harpool's original PR, and I think mimic Postgres' own concept of what the types contain. I'd be wary of strings at the WIT level because they're so flexible and their semantics so ambiguous, although I know Postgres allows e.g. time('11:22:33') in SQL so maybe it would be fine. In my mind I'd expect adapters so users would pass things like Rust chrono::NaiveDate or JS Date rather than raw WIT fields but maybe I am overthinking it? I am not sure
There was a problem hiding this comment.
time('11:22:33') this was the precise example i was thinking about that shows how permissive PG is and how it can parse many formats. By adapter do you mean SDKs on top of the WIT? Regardless, I am happy to leave this as is. We can always add a type with a string parameter if there are user requests
There was a problem hiding this comment.
Yes, SDKs or helper functions or whatever. Like Rust's native Postgres driver accepts chrono::NaiveDate for dates, so it would be good to offer something like that.
| @@ -59,21 +65,26 @@ impl<C: Client> InstanceState<C> { | |||
| } | |||
| } | |||
|
|
|||
| #[async_trait] | |||
| impl<C: Send + Sync + Client> v2::Host for InstanceState<C> {} | |||
| fn v2_params_to_v3( | |||
There was a problem hiding this comment.
@itowlson have you tried running a v2 app on this v3 interface as a sanity check?
There was a problem hiding this comment.
I've run an application using the Rust SDK 3 (which sits over the v2 interfaces) and it worked fine. It wasn't a very comprehensive test though, I fear...
| | v2::rdbms_types::ParameterValue::Uint16(_) | ||
| | v2::rdbms_types::ParameterValue::Uint32(_) | ||
| | v2::rdbms_types::ParameterValue::Uint64(_) => { | ||
| return Err(v2::rdbms_types::Error::ValueConversionFailed( |
There was a problem hiding this comment.
Is this a breaking change to v2 apps? Or were we throwing errors for this previously (in 2.0)?
There was a problem hiding this comment.
Yes, v2 apps also failed, but later, during the "convert rdbms-types type to Postgres type" stage (
spin/crates/factor-outbound-pg/src/client.rs
Line 123 in 3eaba5f
If you or other folks feel that's an unreasonable judgment, I can look at other ways to tackle this! But I'm definitely keen to keep unsigned types out of the v3 interface.
There was a problem hiding this comment.
Failing earlier sounds good to me and i agree that folks relying on a specific error does not seem to outweigh the benefit of removing unsigned types
itowlson
force-pushed
the
db-date-time-2
branch
from
7314253
to
c5eb166
Compare
Signed-off-by: itowlson <[email protected]>
itowlson
force-pushed
the
db-date-time-2
branch
from
c5eb166
to
058aae2
Compare
This is based on @tyler-harpool's work in #2865. I've migrated it to the new v3 world and the new package structure, and implemented it into the Postgres factor.
So far this has not yet been tested (the kindest thing that can be said about it is that it compiles... on my machine). I'm putting the draft PR up so that Rust and Wasm gurus can suggest easier/terser ways of doing things.@tyler-harpool, I hope you don't feel this is treading on your toes, but some of the migration stuff was a bit tricky and tedious, and I really wanted to get a handle on what implementing an interface upgrade was going to look like. I apologise in advance if I have caused any upset. Also if you have any feedback on the way I've implemented the new types (see the
to_sql_parameterfunction inclient.rs) then that would be super useful as I'm not very au fait with the specifics of Postgres types! Thanks!