[Python] Add support for +/- infinity `datetime` objects #8294

Tishj · 2023-07-18T12:23:35Z

This PR fixes #8143 and #8214

To fix 8214, we now add double quotes around the identifier when performing the internal lookup

To fix 8143 we now produce datetime.datetime.max and datetime.date.max values for INFINITY timestamps and dates respectively.
For -INFINITY we produce the .min variant of the datetime object.

This also adds support for reading those values as INFINITY and -INFINITY

…ption instead

hawkfish · 2023-07-18T17:54:24Z

tools/pythonpkg/src/native/python_objects.cpp

@@ -405,12 +448,31 @@ py::object PythonObject::FromValue(const Value &val, const LogicalType &type) {
 	case LogicalTypeId::TIMESTAMP_TZ: {
 		D_ASSERT(type.InternalType() == PhysicalType::INT64);
 		auto timestamp = val.GetValueUnsafe<timestamp_t>();
+
+		InfinityType infinity = InfinityType::NONE;


I'm slightly concerned with the existing code that we can overflow Python's more limited date range. Is that handled elsewhere?

I don't think we do anything to prevent that no

hawkfish · 2023-07-18T17:56:14Z

tools/pythonpkg/tests/fast/api/test_attribute_getter.py

+    def test_getattr_spaces(self):
+        rel = duckdb.sql('select 42 as "hello world"')
+        assert rel['hello world'].fetchall()[0][0] == 42


Not an issue, but If you use .editorconfig, it will add trailing line feeds to Python files.

We should probably run a formatter/linter on Python code in the repo at this point, @Mause any thoughts on that?

I think Elliana suggested we should use black to format the python code

I actually have a pr with that I was looking to submit once the big python PR's had been merged (because otherwise we'll end up a bunch of conflicts)

Mytherin

Thanks for the PR! LGTM. Perhaps @Mause wants to have a look as well?

Mause

Looks good! Just a couple of minor comments

Mause · 2023-07-19T13:15:01Z

tools/pythonpkg/tests/fast/api/test_attribute_getter.py

+    def test_getattr_spaces(self):
+        rel = duckdb.sql('select 42 as "hello world"')
+        assert rel['hello world'].fetchall()[0][0] == 42


I actually have a pr with that I was looking to submit once the big python PR's had been merged (because otherwise we'll end up a bunch of conflicts)

Mause · 2023-07-19T13:15:24Z

tools/pythonpkg/src/native/python_objects.cpp

@@ -434,6 +496,12 @@ py::object PythonObject::FromValue(const Value &val, const LogicalType &type) {

 		auto date = val.GetValueUnsafe<date_t>();
 		int32_t year, month, day;
+		if (!duckdb::Date::IsFinite(date)) {
+			if (date == date_t::infinity()) {
+				return py::module::import("datetime").attr("date").attr("max");


Not caching the min/max values?

I don't think our import cache does anything other than remove a few extra string comparisons.
We haven't measured this, but from working with the code for a while I am confident pybind caches the imports itself

Mause · 2023-07-19T13:17:21Z

tools/pythonpkg/tests/fast/types/test_datetime_datetime.py

+                'TIMESTAMP_MS',
+                'TIMESTAMP_S'
+        ]:
+            # This query is not supported in core duckdb but doesn't deterministically produce an error


Any idea why it doesn't deterministically error?

No clue, I think it has to do with an earlier under/overflow from an unchecked expression.
I noticed INFINITY and -INFINITY are not really implemented for TIMESTAMP_S/MS/NS, as I am having to check for the truncated versions of the timestamp_t::infinity value.

struct timestamp_ms_t : public timestamp_t { // NOLINT static constexpr timestamp_t infinity() { return timestamp_t(int64_t(NumericLimits<int64_t>::Maximum() / 1000) * 1000); } static constexpr timestamp_t ninfinity() { return timestamp_t(int64_t(-NumericLimits<int64_t>::Maximum() / 1000) * 1000); } };

So it might be a result of that

Mytherin · 2023-07-21T10:43:36Z

src/include/duckdb/common/types/timestamp.hpp

+};
+struct timestamp_ms_t : public timestamp_t { // NOLINT
+	static constexpr timestamp_t infinity() {
+		return timestamp_t(int64_t(NumericLimits<int64_t>::Maximum() / 1000) * 1000);


Let's always use timestamp_t::infinity.

@hawkfish could you perhaps have a look at this, it looks like there is an issue when casting infinity to non-microsecond timestamp types, e.g.:

D select 'infinity'::timestamp_ms as ts; ┌───────────────────────────┐ │ ts │ │ timestamp_ms │ ├───────────────────────────┤ │ 294247-01-10 04:00:54.775 │ └───────────────────────────┘

… they're bugged currently

…nfinity

Mytherin · 2023-07-27T08:59:35Z

Thanks!

Tishj added 5 commits July 11, 2023 18:20

deal with infinity in fetchall

c1cc0d4

rename because there already exists a test called 'test_date.py'

2b27fb6

add roundtripping tests for datetime.date/datetime.max/min values

421476d

duckdb.Error fails on CI linux for some reason, trying ConversionExce…

bf8a915

…ption instead

fix undeterministic error in test

7e3fc28

hawkfish approved these changes Jul 18, 2023

View reviewed changes

Mytherin requested a review from Mause July 19, 2023 12:21

Mytherin approved these changes Jul 19, 2023

View reviewed changes

Mause reviewed Jul 19, 2023

View reviewed changes

Mytherin reviewed Jul 21, 2023

View reviewed changes

use the import cache, get rid of the code for non-usecond timestamps,…

7cb3bce

… they're bugged currently

github-actions bot marked this pull request as draft July 21, 2023 12:09

Tishj marked this pull request as ready for review July 23, 2023 15:20

Merge remote-tracking branch 'upstream/master' into python_datetime_i…

10f798a

…nfinity

github-actions bot marked this pull request as draft July 24, 2023 14:14

Tishj marked this pull request as ready for review July 24, 2023 14:55

Mytherin merged commit 2138aa9 into duckdb:master Jul 27, 2023
53 checks passed

ned2 mentioned this pull request Sep 9, 2023

[Python] Column names with spaces failing to work as identifiers #8214

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] Add support for +/- infinity `datetime` objects #8294

[Python] Add support for +/- infinity `datetime` objects #8294

Tishj commented Jul 18, 2023 •

edited

hawkfish Jul 18, 2023

Tishj Jul 18, 2023

hawkfish Jul 18, 2023

Mytherin Jul 19, 2023

Tishj Jul 19, 2023

Mause Jul 19, 2023

Mytherin left a comment

Mause left a comment

Mause Jul 19, 2023

Mause Jul 19, 2023

Tishj Jul 19, 2023

Mause Jul 19, 2023

Tishj Jul 19, 2023

Mytherin Jul 21, 2023

Mytherin commented Jul 27, 2023

[Python] Add support for +/- infinity datetime objects #8294

[Python] Add support for +/- infinity datetime objects #8294

Conversation

Tishj commented Jul 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mytherin left a comment

Choose a reason for hiding this comment

Mause left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mytherin commented Jul 27, 2023

[Python] Add support for +/- infinity `datetime` objects #8294

[Python] Add support for +/- infinity `datetime` objects #8294

Tishj commented Jul 18, 2023 •

edited