Skip to content
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

Add boundary check for converting duckdb date/timestamps to PG dates/timestamps #653

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shah-nirmit
Copy link

Postgres Only Supports date specific date range But DuckDb can return values outside this range , we should detect this and Either error out or pass boundary values.

Fixes #595

@shah-nirmit
Copy link
Author

shah-nirmit commented Mar 7, 2025

@JelteF
These changes throw error if the values are out of bounds , which breaks regression test named test_all_types , IIUC it tests the bounds of all data type in duckdb.

...
not ok 9     - test_all_types                             21 ms
....
1..34
# 1 of 34 tests failed.
postgres=# SELECT * FROM duckdb.query($$ select '4714-11-23 (BC)'::date as date $$);
ERROR:  (PGDuckDB/Duckdb_ExecCustomScan_Cpp) Out of Range Error: The value should be between min and max value (4714-11-24 (BC) <-> 5874897-12-31)
postgres=# SELECT * FROM duckdb.query($$ select '4714-11-24 (BC)'::date as date $$);
     date      
---------------
 4714-11-24 BC
(1 row)
postgres=# SELECT * FROM duckdb.query($$ select '5874897-12-31'::date as date $$);
     date      
---------------
 5874897-12-31
(1 row)

postgres=# SELECT * FROM duckdb.query($$ select '5874898-01-01'::date as date $$);
ERROR:  (PGDuckDB/Duckdb_ExecCustomScan_Cpp) Out of Range Error: The value should be between min and max value (4714-11-24 (BC) <-> 5874897-12-31)

If we dont to throw an error we can use the boundary values for out of range values but we should throw a elog(NOTICE that we are doing this or user could get confused)

Also if you have some tips on debugging across C/C++ boundary using gdb (VS code extension seems to not want to work), that would be really appreciated , was trying to walk throught the query lifecycle with pg_duckdb ?

@JelteF
Copy link
Collaborator

JelteF commented Mar 7, 2025

Thanks for the contribution! Some thoughts:

  1. We need the same thing for timestamp and timestamptz, not just for date. This could be done in a separate PR if you think that's easier.
  2. We should probably also protect conversion the other way around for timestamp and timestamptz, for date it seems like the supported dates in DuckDB are a strict-superset of the supported dates in Postgres.
  3. Throwing an error seems the correct thing to do. That's what both DuckDB and postgres do on out-of-bound dates. For the test_all_types test, Just add the necessary types to the exclude list, and then indeed do what you propose: add separate tests to test the bounds correctly.

@JelteF
Copy link
Collaborator

JelteF commented Mar 7, 2025

Also, do you know if this also fixes this issue? #629

If so we should include that in the tests.

@shah-nirmit
Copy link
Author

Also, do you know if this also fixes this issue? #629

If so we should include that in the tests.

With my changes it throws out of bounds Error

postgres=# select * from duckdb.query($$ select 'infinity'::timestamp, 'infinity'::date $$);
ERROR:  (PGDuckDB/Duckdb_ExecCustomScan_Cpp) Out of Range Error: The value should be between min and max value (4714-11-24 (BC) <-> 5874897-12-31)

@JelteF
Copy link
Collaborator

JelteF commented Mar 7, 2025

With my changes it throws out of bounds Error

Hmm, I guess that's better than returning something wrong. But it would be good if we actually converted infinity/-infinity correctly for dates and timestamps, because both databases support it.

@shah-nirmit shah-nirmit changed the title Add boundary check for converting duckdb date to PG dates Add boundary check for converting duckdb date/timestamps to PG dates/timestamps Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle dates that underflow differently
2 participants