-
Notifications
You must be signed in to change notification settings - Fork 247
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
Postgres date-time types #2890
base: main
Are you sure you want to change the base?
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. |
b025536
to
594892e
Compare
Signed-off-by: itowlson <[email protected]>
594892e
to
7314253
Compare
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.
Just had a few comments to understand how we are handling timezone.
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
binary(list<u8>), | ||
date(tuple<s32, u8, u8>), // (year, month, day) | ||
time(tuple<u8, u8, u8, u32>), // (hour, minute, second, nanosecond) | ||
datetime(tuple<s32, u8, u8, u8, u8, u8, u32>), // (year, month, day, hour, minute, second, nanosecond) |
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.
Should we document in the WIT that UTC is assumed?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we consider letting users provide strings for date and time?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itowlson have you tried running a v2 app on this v3 interface as a sanity check?
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change to v2 apps? Or were we throwing errors for this previously (in 2.0)?
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_parameter
function inclient.rs
) then that would be super useful as I'm not very au fait with the specifics of Postgres types! Thanks!