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.

Sign up for GitHub

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

Jump to bottom

Use local time for column/row headers and computed functions #1074

Merged
merged 9 commits into from
Jun 9, 2020
Merged

Use local time for column/row headers and computed functions #1074

merged 9 commits into from
Jun 9, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Jun 2, 2020

This PR fixes several issues with computed functions and computed columns in perspective-python:

  • the hour_of_day, day_of_week, and month_of_year functions now use local time to perform their calculations. Previously, all calculations were in UTC which could cause issues with datetimes that were on the cusp of one day/month to the next, as well as returning the wrong hour of day when compared with the input column.
  • row and column headers for datetimes (i.e. when a datetime column is pivoted on) are now in local time, consistent with the behavior of datetimes across Perspective:

Screen Shot 2020-06-02 at 6 13 47 PM

  • Additional tests have been added for both features
  • Edits to documentation around datetime handling in the Python guide
  • Fixes a race condition where computed columns created via attribute were created but not preserved after the view changes.
  • Fixes a scrolling issue within Jupyterlab (and other environments where Perspective is both > page width and the page is scrollable vertically) where using the arrow keys to navigate autocomplete would move the entire page.

@sc1f sc1f added bug Concrete, reproducible bugs internal Internal refactoring and code quality improvement documentation Improvements/bugs/changes to documentation Python labels Jun 2, 2020
@sc1f sc1f force-pushed the datetime-fixes branch from ec41903 to f41f4ad Compare June 2, 2020 22:53
@sc1f sc1f force-pushed the datetime-fixes branch from f41f4ad to 8a47108 Compare June 2, 2020 23:43
@andyferris
Copy link

Is it possible to have both behaviours implemented and be able to choose?

@sc1f
Copy link
Contributor Author

sc1f commented Jun 3, 2020

Is it possible to have both behaviours implemented and be able to choose?
@andyferris

Which behaviours are you referring to exactly? For the computed functions, even though it would be possible to preserve the old behaviour of UTC, it just isn't correct when compared with the underlying data:

Before
Screen Shot 2020-06-03 at 10 47 02 AM

After
Screen Shot 2020-06-03 at 10 46 53 AM

The issue is the same with row headers - we don't have an API for controlling their output from userland (they are generated in the C++ part of the codebase), and UTC output would be inconsistent with the underlying data.

texodus
texodus approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This is a fantastic fix.

@andyferris With this change you can present datetime values in whatever timezone and format you choose, either via perspective-viewer-datagrid formatting, or the localized datetime from the perspective API. I think the important take away is that time-zone relativeness in encoding is now preserved, which is very important as perspective data on the server and client may be distributed across timezone. If you have any questions about this or have suggestions for how to improve this API, I encourage you to open a new Issue to describe it - we are very interested in feedback on this topic!

@texodus texodus merged commit e083aa8 into master Jun 9, 2020
@texodus texodus deleted the datetime-fixes branch June 9, 2020 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@texodus texodus texodus approved these changes

Assignees
No one assigned
Labels
bug Concrete, reproducible bugs documentation Improvements/bugs/changes to documentation internal Internal refactoring and code quality improvement Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sc1f @andyferris @texodus @finos-admin