Normalize numeric dtypes when loading/reading from IO #470
Conversation
The idea is to keep the numeric dtypes only using Int64 for integers or Float64 for floats.
There was a problem hiding this comment.
This is great @philss! The change is still missing for binary data however:
Perhaps future misses like that could be avoided by increasing code sharing between the functions a bit?
native/explorer/src/dataframe/io.rs
Outdated
| .with_columns(columns) | ||
| .with_projection(projection) | ||
| .finish()?; | ||
| Ok(ExDataFrame::new(df)) | ||
|
|
||
| let normalized_df = normalize_numeric_dtypes(&mut df)?; | ||
|
|
||
| Ok(ExDataFrame::new(normalized_df)) |
There was a problem hiding this comment.
Could this part be factored out into a function by using the SerReader trait so that future changes to the import through of SerReader do not miss any of the functions?
There was a problem hiding this comment.
Thanks for the suggestion! I'm not sure if I understood correctly, but I added a function to abstract the "finish" process, which is more clear now. Please check again :)
There was a problem hiding this comment.
I'm merging this in order to add to the new release. Please fell free to open a PR if you think this can be improved.
native/explorer/src/lazyframe.rs
Outdated
|
|
||
| let normalized_df = crate::dataframe::normalize_numeric_dtypes(&mut df)?; | ||
|
|
||
| Ok(ExDataFrame::new(normalized_df)) |
There was a problem hiding this comment.
Doing this change here is too late because all operations in the lazy frame would then act on i32, u32 and so on. We also need to do them immediately after reading the file/binary.
There was a problem hiding this comment.
I see. I don't know yet how to apply this change for a LazyFrame, so I decided to revert and keep it just for the eager frame. I think I'm going to open an issue for that and tackle after the release. WDYT?
I forgot the most important 😅 |
The idea is to keep the numeric dtypes only using Int64 for integers or Float64 for floats.
I tested with some parquet files from this project: https://github.com/PRQL/prql-query and they all worked fine.
Closes #468