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

feat(py/client-ticking): Add support for LocalDate and LocalTime #6106

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

kosak
Copy link
Contributor

@kosak kosak commented Sep 22, 2024

This PR adds support for LocalDate and LocalTime to pydeephaven/ticking.

Note this PR depends on #6105, so that one should be merged first.

It also depends on #6137, which also needs to be merged first.

I also add a comment to pydeephaven/README.md to remind people to make a venv.

Also note that this is a fix for #6044

@kosak kosak added NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Sep 22, 2024
@kosak kosak self-assigned this Sep 22, 2024
Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

The changes LGTM but I'd like to see a simple test case for the new types.

@chipkent
Copy link
Member

I second @jmao-denver 's comment.

@kosak kosak force-pushed the kosak_py-ticking-localdate-localtime branch from fa428f8 to 2a7711f Compare September 26, 2024 04:05
@jmao-denver jmao-denver self-requested a review September 26, 2024 14:50
Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

The test looks good. It doesn't need to cover uploading arrow data to the server (doPut). I wonder though if the C++ client needs to support doPut and if that's been tested. I am asking because the R client depends on it.

@jmao-denver jmao-denver self-requested a review September 26, 2024 15:02
Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

LGTM

@kosak kosak merged commit b404621 into deephaven:main Sep 26, 2024
19 checks passed
@kosak kosak deleted the kosak_py-ticking-localdate-localtime branch September 26, 2024 16:28
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2024
@kosak
Copy link
Contributor Author

kosak commented Sep 27, 2024

The test looks good. It doesn't need to cover uploading arrow data to the server (doPut). I wonder though if the C++ client needs to support doPut and if that's been tested. I am asking because the R client depends on it.

There are two answers to this question. The first is that if people are using our ColumnSource style interface, then yes, C++ supports that. In our C++ client we have two new types: LocalDate and LocalTime, and we have a test that shows pushing a vector of those to the server: https://github.com/deephaven/deephaven-core/blob/main/cpp-client/deephaven/tests/src/table_test.cc

It's also possible that R is using the Arrow doPut call to push data to Deephaven directly. If so, that's outside the scope of this change and should already work.
I think I need to talk to @alexpeters1208 to make sure this provides what he needs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants