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

fix(rust, python): allow extract of numeric from str AnyValue #13865

Merged
merged 7 commits into from
Jan 23, 2024

Conversation

mcrumiller
Copy link
Contributor

@mcrumiller mcrumiller commented Jan 19, 2024

Resolves #13862.

Not completely sure this is the way to go, but the reason the error is occuring is because the frame creation using a series of dicts involves adding records row-by-row to the frame. The result is the use of the AnyValueBuffer::add_fallible() which in turn calls AnyValue::extract() on the read value. If the read value is str and the target is int, AnyValue::extract would panic. This adds a branch which tries to cast the string to an int, so the conversion will succeed for valid ints.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Jan 19, 2024
@ritchie46
Copy link
Member

I don't think we should do this this way. See my comment in issue. In any case the i64 is completely arbitrary. It could also be a float.

@mcrumiller
Copy link
Contributor Author

Ok I totally agree that the implementation is bad, unsure how to align with Series/Expr though.

As an aside, I am trying to figure out why pl.lit("1").cast(pl.UInt8) succeeds when the AnyValue::cast function returns an Error. It looks like the optimizer is somehow taking a different path which doesn't error:

pl.select(pl.lit("1").cast(pl.UInt8))  # succeeds

@ritchie46
Copy link
Member

Yes, it goes though the Series cast function. The AnyValue is only meant a simple optimization. When stuff gets more opinionated or complex it should go through the dedicated function for columns.

@mcrumiller
Copy link
Contributor Author

Ok. Should I close this, or do we still want to address the associated issue?

@ritchie46
Copy link
Member

I think this one can be closed. It would be worth it if we got a better error message for this one.

@ritchie46
Copy link
Member

Hmm.. Sorry for the confusion @mcrumiller. I thought we were somewhere else in the codebase.

This might work if we get more information on how to parse the string. E.g. do we need to parse as either f64 or i128 (we also must fit u64).

@mcrumiller mcrumiller changed the title fix(rust, python): allow extract of int from str AnyValue fix(rust, python): allow extract of numeric from str AnyValue Jan 22, 2024
@ritchie46 ritchie46 merged commit f4d46ac into pola-rs:main Jan 23, 2024
22 checks passed
r-brink pushed a commit to r-brink/polars that referenced this pull request Jan 24, 2024
@mcrumiller mcrumiller deleted the from-dict-anyvalue branch January 26, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

schema_overrides is broken when converting from string to numeric
2 participants