-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Date/datetime filter autocompletion, new timezone test suite for JS #1282
Conversation
Summary of datetime handling in
The issue with |
bfa9b02
to
864582f
Compare
50df4c9
to
32b38c9
Compare
120cdd2
to
9144258
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for the PR! Reviewed offline.
WIP: add US and UK datestrings Remove UK locale string parsing - too much ambiguity
Add new timezone test module don't run tz test twice
convert date and datetime filters to new Date(), fix formatting for date filters refactor test_js script, fix tz tests Add a ton of tests
24f09a9
to
9d639ae
Compare
…an `<input>` last
9d639ae
to
9c88df7
Compare
These tests flapped because some "filter completion" queries were taking longer than the cursor blink timeout. As a fix, to any test which calls |
This PR fixes the datetime filtering issues specified in #1242, and clarifies the behavior of
perspective.js
with date and datetime columns by introducing a test suite that runs in theUS/Eastern
timezone.Previously, the behavior of datetime filters (especially for
==
) was inconsistent and partially broken:==
filter that tries to match12/31/2020 12:30:00 PM", for example, would require the string
12/31/2020 5:30:00 PM" in order to actually match the correct value inside the engine, which stores the values as POSIX timestamps.This PR implements the following fixes to address the issue:
perspective.js
, all filters fordate
anddatetime
columns are converted tonew Date()
before being passed into the engine. This allows Perspective to treat the value as local time and call thegetTime
API for the POSIX timestamp, meaning that date/datetime filters will "match" in the UI.perspective-viewer
,date
anddatetime
filters now have an autocomplete, which allows users to narrow down the exact filter value, and provides them with a format that is guaranteed to work with the engine:A threshold of 100,000 rows has been added so that the filter does not try to materialize too many values to search through, which can happen on large datasets with large numbers of unique datetimes/strings. This does not change the filter behavior—only the suggestions provided by the autocomplete.
Additionally, while behavior regarding timezones and various date container values are explicitly specified for
perspective-python
, they are less clear forperspective.js
. This PR adds a test suite that runs in theUS/Eastern
timezone in order to assert that datetime behavior is clear and correctly implemented when UTC offsets are involved, similar to theperspective-python
test suite targeting the same issue.Caveats
In order for US locale strings to be treated as valid by the filter validator, the locale string format must be added to the parser format definition in
arrow_csv.cpp
. This has the side effect of parsing US locale strings as part of the dataset, where I found an issue withstrptime
in the C STL - when given the US locale string format ("%m/%d/%Y, %I:%M:%S %p"), it parses "12:00:00 AM" as "12:00:00 PM", which is incorrect. This seems to be an issue withstrptime
when compiled in Emscripten; the Python library does not have the same issue. Users should pass in date values asDate()
objects, but there can be cases where strings are being passed in and this parsing error occurs.