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

Support ZonedDateTime columns in plots #4528

Merged
merged 2 commits into from
Sep 22, 2023
Merged

Conversation

chipkent
Copy link
Member

Support ZonedDateTime columns in plots.

Resolves #4466

@chipkent chipkent added bug Something isn't working plotting labels Sep 20, 2023
@chipkent chipkent self-assigned this Sep 20, 2023
mofojed
mofojed previously approved these changes Sep 20, 2023
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Tested with the snippet in the ticket:

from deephaven import empty_table
from deephaven.plot import Figure

t = empty_table(1).update_view(formulas=['t=(ZonedDateTime)null', 'y=(long)null'])
plot = Figure().plot_xy(t=t, x="t", y="y", series_name="sample").show()

Comment on lines 412 to 415
public double getDouble(int i) {
final Instant value = DateTimeUtils.toInstant((ZonedDateTime) getDataColumn().get(i));
return value == null ? Double.NaN : DateTimeUtils.epochNanos(value);
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this even get used? I have the impression that web is snapshotting data directly rather than via any Plot code. Which is good, because this is a pretty sub-optimal way to fetch bulk data.

Copy link
Member Author

@chipkent chipkent Sep 22, 2023

Choose a reason for hiding this comment

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

Here is some test code I have been using.

from deephaven import time_table
from deephaven.time import to_j_time_zone
from deephaven.plot.figure import Figure

tz = to_j_time_zone("ET")
t = time_table("PT00:00:01").update(["X=toZonedDateTime(Timestamp,tz)","Y=i"])
m = t.meta_table
p = Figure().plot_xy(series_name="Test", t=t, x="X", y="Y").show()

In the 0.28.1 demo, this fails with:

java.lang.UnsupportedOperationException: Unsupported numeric data type: columnName=X type=class java.time.ZonedDateTime

The equivalent Groovy is passing in my branch code.

tz = timeZone("ET")
t = timeTable("PT00:00:01").update("X=toZonedDateTime(Timestamp,tz)","Y=i")
m = t.meta()
p = figure().plot("Test", t, "X", "Y").show()

I will disable and test the code again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed that commenting out the new code results in an exception: java.lang.UnsupportedOperationException: Unsupported numeric data type: columnName=X type=class java.time.ZonedDateTime

Copy link
Member Author

Choose a reason for hiding this comment

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

A deep dive indicates that some, but not all of this code is called. The methods to get type are called, but the getDouble methods are not called. Removal of those functions should be part of a more comprehensive refactor and not this PR.

@chipkent chipkent merged commit 603bd69 into deephaven:main Sep 22, 2023
10 checks passed
@chipkent chipkent deleted the 4466_plot_zdt branch September 22, 2023 20:12
@github-actions github-actions bot locked and limited conversation to collaborators Sep 22, 2023
@deephaven-internal
Copy link
Contributor

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/3229
Reference: https://github.com/deephaven/deephaven.io/issues/3230

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working DocumentationNeeded plotting ReleaseNotesNeeded Release notes are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plotting doesn't respect ZonedDateTime as a numeric column
4 participants