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

Support time dtype #487

Merged
merged 5 commits into from
Jan 29, 2023
Merged

Support time dtype #487

merged 5 commits into from
Jan 29, 2023

Conversation

sasikumar87
Copy link
Contributor

  1. I'm new to rust, pls help me to find the cause of the failing tests.
  2. What existing Series functions can support time dtype ?

@josevalim
Copy link
Member

I would look at where Date and DateTime are supported. :) It is probably going to be a subset of that. :)

philss
philss reviewed Jan 27, 2023
View reviewed changes
Copy link
Contributor

@philss philss left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I added two points related to the same thing.

I cannot understand yet why it's getting an error.

But mostly the code looks good!

native/explorer/src/expressions.rs Outdated Show resolved Hide resolved
lib/explorer/polars_backend/expression.ex Outdated Show resolved Hide resolved
@sasikumar87 sasikumar87 marked this pull request as ready for review January 28, 2023 08:57
@sasikumar87 sasikumar87 requested a review from philss January 28, 2023 08:58
philss
philss approved these changes Jan 28, 2023
View reviewed changes
Copy link
Contributor

@philss philss left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

The only thing I would suggest is to add more tests to the series_test.exs file. We are lacking some tests there, because in the past we rely only on doctests. But now we want to have a better test suite.

This could be in another PR, but would be cool if you could add tests to the functions you changed (like min/max/cummulative_min, etc).

@josevalim
Copy link
Member

Let's add the tests now so we build on the practice of adding more tests. Can you please do that @sasikumar87? thank you ❤️

@sasikumar87
Copy link
Contributor Author

@philss @josevalim sure.

I've already added doctests for impacted functions. Should I remove and add them to series_test.exs or keep both?

@josevalim josevalim merged commit 74821c6 into elixir-explorer:main Jan 29, 2023
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@philss
Copy link
Contributor

philss commented Jan 29, 2023

@sasikumar87 please keep both.

@josevalim
Copy link
Member

Keep both please! I thought this was good to merge but the improved coverage will be welcome! ❤️

@sasikumar87 sasikumar87 deleted the support-time-dype branch January 30, 2023 04:49
liamdiprose pushed a commit to liamdiprose/explorer that referenced this pull request Feb 16, 2023
@filmor
Merge pull request elixir-explorer#487 from filmor/make-resource-binary
d89a3d1 
Implement ResourceArc::make_binary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philss philss philss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sasikumar87 @josevalim @philss