Skip to content

Conversation

shah-nirmit
Copy link
Contributor

@shah-nirmit shah-nirmit commented Mar 7, 2025

Postgres and DuckDB support different date and timestamp ranges. This starts to error out when either one receives a value outside of this range. This also starts converting special values for the infinity and -infininity correctly, because these are also not the same between the two databases.

Fixes #595
Fixes #612

@shah-nirmit
Copy link
Contributor 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
Contributor 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
@JelteF
Copy link
Collaborator

JelteF commented Mar 10, 2025

Thanks for the work. This looks almost good, left some mostly cosmetic feedback. CI is also failing due to a compiler error/warning.

@JelteF JelteF mentioned this pull request Mar 10, 2025
@shah-nirmit
Copy link
Contributor Author

Had to use PG_INT64* Macros for timestamp as specific macros used before were introduces in specific majors only. This will also avoid ugly pg version checks...

@shah-nirmit shah-nirmit requested a review from JelteF March 18, 2025 04:28
Copy link
Collaborator

@JelteF JelteF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the datum conversion functions for the correct types (even though they are functionally equivalent). Other than that this is now good to merge.

shah-nirmit and others added 6 commits March 18, 2025 14:03
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
@JelteF JelteF merged commit ebef8c8 into duckdb:main Mar 18, 2025
5 checks passed
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