ENH: improve support for datetime columns #486
Conversation
theroggy
changed the title
…ith-naive-datetimes-with-arrow
theroggy
changed the title
theroggy
changed the title
theroggy
changed the title
theroggy
changed the title
theroggy
changed the title
There was a problem hiding this comment.
@theroggy thanks for further looking into this!
I do have some doubts about how much effort we should do to cover corner cases and what the desired default behaviour should be, see my comments below.
| # if object dtype, try parse as utc instead | ||
| if res.dtype == "object": | ||
| try: | ||
| res = pd.to_datetime(ser, utc=True, **datetime_kwargs) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
From your top post explanation:
Add support to read mixed timezone datetimes. These are returned in an object column with Timestamps.
First, I don't think this will work with upcoming pandas 3.x (we are suppressing the warning above about mixed timezones going to raise unless passing utc=True, and that you have to use apply and datetime.datetime.strptime instead to get mixed offset objects)
(but the tests are also passing, so maybe I am missing something)
Second, a column of mixed offset objects is in general not that particularly useful .. So changing this behaviour feels like a regression to me. I understand that we might want to provide the user the option to get this, but by default, I am not sure.
There was a problem hiding this comment.
From your top post explanation:
Add support to read mixed timezone datetimes. These are returned in an object column with Timestamps.
First, I don't think this will work with upcoming pandas 3.x (we are suppressing the warning above about mixed timezones going to raise unless passing
utc=True, and that you have to useapplyanddatetime.datetime.strptimeinstead to get mixed offset objects) (but the tests are also passing, so maybe I am missing something)
Yes, I saw. Do you know what the rationale is that in pandas 3 people are being forced to use a more inefficient way (apply) to get to your data?
Second, a column of mixed offset objects is in general not that particularly useful .. So changing this behaviour feels like a regression to me. I understand that we might want to provide the user the option to get this, but by default, I am not sure.
For starters, to be clear, this is only relevant for mixed timezone data. Data saved in naive or UTC timestamps should just stay "regular" pandas datetime columns.
For the case of mixed timezone data, it depends on what you want to do with the datetime data. If it is just to look at it/show/keep it as it is part of the table data, the Timestamps look just fine to me. If you really want to do "fancy stuff" with the datetimes it will in pandas indeed be more convenient for some things to transform them into e.g. UTC datetimes to get a datetime column instead of an object column.
Regarding default behaviour, it feels quite odd to me to transform data by default to a form where information (the original time zone) is lost. Also because when you save the data again, it will then be saved as UTC as well, so also: the timezone information will be lost.
To me, the other way around is more logical: by default you don't loose data. If you want to do "fancy stuff" with a datetime column that contains mixed timezone data, you convert it to e.g. UTC, typically in an extra column, because most likely you will want to keep the timezone information again when saving.
pyogrio/geopandas.py
Outdated
| elif col.dtype == "object": | ||
| # Column of Timestamp objects, also split in naive datetime and tz offset | ||
| col_na = df[col.notna()][name] | ||
| if len(col_na) and all(isinstance(x, pd.Timestamp) for x in col_na): |
There was a problem hiding this comment.
I am a bit hesitant to add custom support for this, exactly given that it is not really supported by pandas itself, do we then need to add special support for it?
Right now, if you have an object dtype column with timestamp columns, they already get written as strings, which in the end preserves the offset information (in the string representation).
It might read back as strings (depending on the file format), but at that point the user can handle this column as they see fit.
There was a problem hiding this comment.
If they are written as strings to the output files without the proper metadata, it depends on format to format if they will be recognized as datetimes when read. For text files they will typically be recognized as datetime as the data types are "guessed" when the file is read (e.g. geojson), for files like .fgb and .gpkg they won't be recognized as the file metadata will be wrong.
That's not very clean, and as it is very easy to solve I don't quite see the point of not supporting it properly?
pyogrio/geopandas.py
Outdated
| elif isinstance(dtype, pd.DatetimeTZDtype): | ||
| # Also for regular datetime columns with timezone mixed timezones are | ||
| # possible when thera is a difference between summer and winter time. | ||
| df[name] = col.apply(lambda x: None if pd.isna(x) else x.isoformat()) | ||
| datetime_cols.append(name) |
There was a problem hiding this comment.
Is this needed for properly typed datetime64 columns?
What does GDAL do with those values? Write as the UTC value? And with this change it will write it, still as a datetime (because of adding the metadata?), but with offset?
FWIW, if we need to do this, you can do df[name].astype(str) to avoid the apply
There was a problem hiding this comment.
Is this needed for properly typed datetime64 columns? What does GDAL do with those values? Write as the UTC value?
It is actually a bit weird. Based on what I tested, when the column is in UTC time zone, the data is written correctly to the file. If the column has another time zone it is simply dropped and the naive times are written. This is the case for both GPKG and e.g. .geojson.
Hence, naive and UTC times can be written via a native arrow datetime column, but in the other cases it needs to be written via a sidestep to a string column.
There is a TIMEZONE option that can be specified in the GDAL arrow code path... but it is only on layer level, so not per column so thats not super useful either. Based on a quick test it also didn't seem to work for timezones like "CET".
And with this change it will write it, still as a datetime (because of adding the metadata?), but with offset?
With this change offsets will be correctly written as timestamp, indeed because of the custom arrow metadata being added and interpreted by GDAL from GDAL 3.11 (OSGeo/gdal#11213)
FWIW, if we need to do this, you can do
df[name].astype(str)to avoid theapply
Interesting! Note however that df[name].astype(str) outputs "... ..." instead of "...T...", so no strictly valid ISO... strings. But, astype gives signifficantly better performance (2 sec instead of 12 sec for nz buildings)... and probably not that important... so I changed it. If we rather want to have "...T...", we can add .str.replace(" ", "T") for GDAL < 3.11, that only adds 0.5 sec.
I used df[name].astype("string"), otherwise None/NAT is also cast to a string.
pyogrio/tests/test_geopandas_io.py
Outdated
| if use_arrow and ext == ".gpkg" and __gdal_version__ < (3, 11, 0): | ||
| pytest.skip("Arrow datetime handling improved in GDAL >= 3.11") |
There was a problem hiding this comment.
What is not yet working for the case with no tz for GPKG?
There was a problem hiding this comment.
Datetimes in a GPKG without timezone are now interpreted as being UTC. So a naive time of 05:00 h is interpreted as 05:00 UTC.
This is one of the issues listed in #487 (comment)
…ith-naive-datetimes-with-arrow
- Test result < GDAL 3.11 instead of skipping - Add UTC test - ...
theroggy
changed the title
This reverts commit e35c356.
Needs to be astype"string") instead of astype(str) to support nan values
This PR improves support for datetime columns, mainly in
read_dataframeandwrite_dataframe:use_arrow, naive datetimes (no timezone) were interpreted as being UTC. So a naive time of 05:00 h was interpreted as 05:00 UTC.use_arrow, for datetime columns with a timezone the timezone was dropped, so 05:00+5:00 was read as 05:00.use_arrow, for datetime columns with any timezone but UTC, the timezone was dropped, so 05:00+5:00 was written as 05:00 (a naive datetime), for all filetypes.use_arrow, don't convert/represent them as being in UTC time if they have another timezone offset in the dataset.use_arrow, the fixes typically require GDAL >= 3.11 (OGRLayer::GetArrowStream(): add a DATETIME_AS_STRING=YES/NO option OSGeo/gdal#11213).Resolves #487