search

KxSystems / rkdb

R client for kdb+
https://code.kx.com/q/interfaces
Apache License 2.0
41 stars 30 forks source link

default timezones set on datetime #63

Open ralmgren1 opened 3 years ago

ralmgren1 commented 3 years ago

I would like to propose modifying src/common.c to remove line 380 in function from_datetime_kobject()

settimezone(result,"GMT");

and also removing or commenting the entire function settimezone() at lines 66-74 since that is the only place this function is used. Here is the reason: This function returns an R POSIXct object. In R, such an object is always in UTC. It may optionally include a tzone attribute, indicating how it would prefer to be displayed; without that attribute it is displayed by default in the machine local timezone though any different display timezone can also be specified. Kdb+ datetime objects have no such property, so the interface should not add this information. If the Kdb+ datetime is not UTC, then the conversion will simply be incorrect, and adding the label does not fix it. R Sys.time() returns local time with no default time zone. It would be useful if execute(db,'.z.z') also had no default time zone. At least in older versions of R, warnings were produced if arithmetic were done between POSIXct objects with different default time zones. Finally, "GMT" is an obsolete specification; the modern usage is "UTC". To summarize: attaching a default time zone to POSIXct objects returned by rkdb is unnecessary, is an inaccurate reflection of what is in the underlying data table, and is a potential source of confusing errors. It would be simpler to remove that code.

cmccarthy1 commented 3 years ago

Thanks for the suggestion, I'm going to implement this change over the next couple of days and rerelease the interface. I think there may be some issues with back compatibility but I believe your analysis is correct and the appropriate change is to remove the timezone setting. I'll add a comment below once a PR is in for this.