TIME type duckdb/postgres conversion #627
Conversation
There was a problem hiding this comment.
Thanks for working on this. This is a nice start. This is only implementing the DuckDB to Postgres conversion currently, but I think this should also implement the Postgres to DuckDB conversion. Then you could also add some tests to test/regression/sql/type_support.sql and test/regression/sql/array_type_support.sql.
src/pgduckdb_types.cpp
Outdated
| Datum pg_time = | ||
| DirectFunctionCall3(time_in, CStringGetDatum(value_str.c_str()), ObjectIdGetDatum(TIMEOID), Int32GetDatum(-1)); | ||
| return pg_time; | ||
| } | ||
|
|
||
| static Datum | ||
| ConvertTimeTzDatum(const duckdb::Value &value) { | ||
| std::string value_str = value.ToString(); | ||
| Datum pg_timetz = DirectFunctionCall3(timetz_in, CStringGetDatum(value_str.c_str()), ObjectIdGetDatum(TIMETZOID), | ||
| Int32GetDatum(-1)); |
There was a problem hiding this comment.
We try to avoid calling into postgres functions when we easily can. Partially to keep conversion overhead low (formatting a number as a string and then parsing it again is not free), but primarily so we can avoid crossing the C<->C++ boundary. Crossing that boundary requires a PostgresFunctionGuard for safety, except for really trivial functions like the SomeTypeGetDatum functions.
In this case it should be fairly simple to convert the internal representations directly to eachother. A Postgres time object is a TimeADT/TimeTzADT which are really (an int64, and a 2 field struct respectively). Let's contstruct that correctly.
There was a problem hiding this comment.
I think your point completely makes sense! In the latest commit, I
- Remove the code for timetz, which I will implement by EOW (mainly due to my time constraint)
- Use the int64 for conversion and add corresponding code/test as I did for INTERVAL
38acb22 to
43b314b
Compare
|
@JelteF Thank you so much for the quick review and help! |
This doesn't implement conversion for the timetz type yet, that will be done in a follow up PR.
Partially addresses #596