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

Do not infer if type has been given #923

Closed
wants to merge 1 commit into from

Conversation

josevalim
Copy link
Member

No description provided.

@josevalim
Copy link
Member Author

This is a WIP. We need to decide if we want to go ahead with this code or not. This assumes that, if the user specifies the dtype, the dtype will be taken precisely as is. We won't try to merge and fix things, which is arguably what we should do, but we need to fix the build.

To fix the build, we need to remember dtypes like {:duration, :milliseconds} must be first read as integers and then cast to the duration (perhaps this could be done directly in the Rust side).

@billylanchantin
Copy link
Contributor

I think going ahead with this approach makes sense. There is a notion in Polars of the "physical" dtype:

If we mirror that on the Elixir side, that may help us pass things to Rust in a safer way. Like, from_list can tell Rust that it's a list of either {:duration, _} or to_physical({:duration, _}).

IDK if that truly makes it easier or not though. I'd have to dig into our code to say more.

@josevalim
Copy link
Member Author

The challenge here is that if you pass from_list([1, 2, 3], dtype: {:duration, ...}), we need to either implement this logic in the backend OR tell the user to pass dtype as integer and then they explicitly cast it to duration. Both are doable for sure.

@josevalim
Copy link
Member Author

Continuing in favor of #928.

@josevalim josevalim closed this Jun 14, 2024
@josevalim josevalim deleted the jv-disable-infer-on-dtype branch June 14, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants