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

Conditional formatting errors when setting an invalid date string #1108

Closed
bmingles opened this issue Feb 24, 2023 · 4 comments · Fixed by #1120
Closed

Conditional formatting errors when setting an invalid date string #1108

bmingles opened this issue Feb 24, 2023 · 4 comments · Fixed by #1120
Assignees
Labels
bug Something isn't working
Milestone

Comments

@bmingles
Copy link
Contributor

bmingles commented Feb 24, 2023

Description
Invalid DateTime strings cause conditional formatting errors that result in an unrecoverable state.

NOTE: #1091 and #1106 should fix related issues. Might be good to wait until those are merged before working on this fix.

Steps to reproduce

  1. Load a table with a DateTime column
  2. Table Options -> Conditional Formatting -> Add New Rule
  3. Select date column, "is exactly", "blah" for condition
  4. Set Style to something other than "No formatting"

Expected results
Invalid DateTime strings should not be applied

  • Done button should be disabled
  • Auto formatting should not be applied
  • No error should be shown

Actual results
Formatting is auto-applied for invalid date format and a "Unable to open table. Error" is shown

Versions

Engine Version: 0.22.0
Web UI Version: 0.30.1
Java Version: 17.0.6
Barrage Version: 0.5.0

@bmingles bmingles added bug Something isn't working triage Issue requires triage labels Feb 24, 2023
@bmingles
Copy link
Contributor Author

bmingles commented Feb 27, 2023

@dsmmcken it looks like we do support shorter date time formats as long as they have the TZ.
e.g. 2023-02-23 NY works fine.

Do we want to keep this restriction, or should we default to the local time zone if none is provided?

Also, it looks like not all timezone suffixes work.
e.g. copying the unformatted cell value of
2021-09-22T12:00:11.949000000 EDT fails, whereas
2021-09-22T12:00:11.949000000 NY works

@bmingles
Copy link
Contributor Author

bmingles commented Mar 1, 2023

It looks like the Java code supports a discreet set of timezone codes. In io.deephaven.time.TimeZone, we have the following enum:

TZ_AL(DateTimeZone.forID("America/Anchorage")),
TZ_AT(DateTimeZone.forID("Canada/Atlantic")),
TZ_BT(DateTimeZone.forID("America/Sao_Paulo")),
TZ_CE(DateTimeZone.forID("Europe/Berlin")),
TZ_CH(DateTimeZone.forID("Europe/Zurich")),
TZ_CT(DateTimeZone.forID("America/Chicago")),
TZ_ET(DateTimeZone.forID("America/New_York")),
TZ_HI(DateTimeZone.forID("Pacific/Honolulu")),
TZ_HK(DateTimeZone.forID("Asia/Hong_Kong")),
TZ_IN(DateTimeZone.forID("Asia/Kolkata")),
TZ_JP(DateTimeZone.forID("Asia/Tokyo")),
TZ_KR(DateTimeZone.forID("Asia/Seoul")),
TZ_LON(DateTimeZone.forID("Europe/London")),
TZ_MN(DateTimeZone.forID("America/Chicago")),
TZ_MOS(DateTimeZone.forID("Europe/Moscow")),
TZ_MT(DateTimeZone.forID("America/Denver")),
TZ_NF(DateTimeZone.forID("Canada/Newfoundland")),
TZ_NL(DateTimeZone.forID("Europe/Amsterdam")),
TZ_NY(DateTimeZone.forID("America/New_York")),
TZ_PT(DateTimeZone.forID("America/Los_Angeles")),
TZ_SG(DateTimeZone.forID("Asia/Singapore")),
TZ_SHG(DateTimeZone.forID("Asia/Shanghai")),
TZ_SYD(DateTimeZone.forID("Australia/Sydney")),
TZ_TW(DateTimeZone.forID("Asia/Taipei")),
TZ_UTC(DateTimeZone.UTC);

When we run an expression such as convertDateTime('2020-01-01 NY'), the TZ_NY, we parse the NY and map to the TZ_NY. There doesn't seem to be support for any timezones outside of these enum values. As far as I can tell, we don't have any way to validate these shorthand timezone enum values in the frontend, so we can't really tell if a given date string is valid until it passes or fails on the backend.

@vbabich
Copy link
Collaborator

vbabich commented Mar 1, 2023

We can use dh.i18n.TimeZone.getTimeZone()

dh.i18n.TimeZone.getTimeZone('TEST'); // throws "java.lang.IllegalArgumentException: Unsupported time zone TEST"
dh.i18n.TimeZone.getTimeZone('NY'); // returns a JsTimeZone object

@vbabich vbabich added this to the February 2023 milestone Mar 1, 2023
@vbabich vbabich removed the triage Issue requires triage label Mar 1, 2023
bmingles added a commit to bmingles/web-client-ui that referenced this issue Mar 1, 2023
@bmingles
Copy link
Contributor Author

bmingles commented Mar 1, 2023

I have added datetime + timezone parsing for frontend validation.

There is an open bug deephaven/deephaven-core#3472 to address an issue with the query engine's current timezone support.

bmingles added a commit to bmingles/web-client-ui that referenced this issue Mar 1, 2023
bmingles added a commit to bmingles/web-client-ui that referenced this issue Mar 2, 2023
bmingles added a commit to bmingles/web-client-ui that referenced this issue Mar 2, 2023
bmingles added a commit that referenced this issue Mar 2, 2023
Added datetime string parsing / validation.

fixes #1108
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants