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

default timezones set on datetime #63

Open
ralmgren1 opened this issue Sep 13, 2021 · 1 comment · May be fixed by #64
Open

default timezones set on datetime #63

ralmgren1 opened this issue Sep 13, 2021 · 1 comment · May be fixed by #64

Comments

@ralmgren1
Copy link

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
Copy link
Contributor

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.

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 a pull request may close this issue.

2 participants