-
Notifications
You must be signed in to change notification settings - Fork 72
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
Support TIMESTAMP_NS, TIMESTAMP_MS & TIMESTAMP_S #534
base: main
Are you sure you want to change the base?
Conversation
Insert statements executed from postgres on tables in motherduck are working as expected for TIMESTAMP_MS & TIMESTAMP_S. -- Tables created in Motherduck
CREATE TABLE timestamps_ms_tbl(a timestamp_ms);
CREATE TABLE timestamps_s_tbl(a timestamp_s);
-- ------------------------------------------------------------------------
-- Insert statements executed from postgres
INSERT INTO timestamps_ms_tbl VALUES('1992-12-12 12:12:12.1989898'::TIMESTAMP);
INSERT INTO timestamps_s_tbl VALUES('1992-12-12 12:12:12.1989898'::TIMESTAMP);
-- Selecting Data from mother duck via postgres
-- -------------------------------------------------------------------
SELECT * FROM timestamps_ms_tbl;
-- Output:
-- 1992-12-12 12:12:12.198
SELECT * FROM timestamps_s_tbl;
-- Output
-- 1992-12-12 12:12:12 Currently working on converting TIMESTAMP_MS selected from duckdb into Postgres TIMESTAMP type |
@JelteF, I tested this against some tables in Motherduck, but I’m not sure how to go about writing regression tests for this. From what I understand, the type_support tests are queries run from within PostgreSQL. Since this PR doesn’t add any new types to Postgres, I’m a bit confused about the best way to proceed. |
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.
I tested this out manually using MotherDuck, and it works well. Code also looks good, but I left a few small comments on how to fix CI.
Regarding how to add similar automated tests. I don't think that's possible indeed. But one thing we can do is at least test the conversion from DuckDB to Postgres by using the duckdb.query
function that was introduced in #531 (which I merged half an hour ago):
SELECT * FROM duckdb.query($$ SELECT '1992-12-12 12:12:12.123456789'::TIMESTAMP_NS as ts $$);
That query works on this branch, but not on main. If you can add similar queries for all the newly supported types then I think we can merge this.
src/pgduckdb_types.cpp
Outdated
int64_t rawValue = value.GetValue<int64_t>(); | ||
|
||
// Handle specific Timestamp unit(sec, ms, ns) types | ||
switch(value.type().id()){ |
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.
needs a default
case to silence the compiler warning
src/pgduckdb_types.cpp
Outdated
// 1 ms = 10^3 micro-sec | ||
rawValue *= 1000; | ||
break; | ||
case duckdb::LogicalType::TIMESTAMP_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.
You need to run make format
to solve the formatting warning.
Cool I'll update this accordingly. |
Draft PR to support TIMESTAMP_NS, TIMESTAMP_MS & TIMESTAMP_S types available in duckdb in postgres.
Fixes #283