-
Notifications
You must be signed in to change notification settings - Fork 12
feat: add a rust crate #369
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
Conversation
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really neat! 🙏
Only a couple minor nitpicks.
My review is rather shallow as I'm not used to this project yet.
Another review would be appreciated!
&& data_len != len | ||
{ | ||
return Err(FastExcelErrorKind::InvalidColumn(format!( | ||
"Column '{name}' has length {data_len} but expected {len}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn build_selected_columns( | ||
use_columns: Option<&Bound<'_, PyAny>>, | ||
) -> FastExcelResult<SelectedColumns> { | ||
use_columns.try_into().with_context(|| format!("expected selected columns to be list[str] | list[int] | str | Callable[[ColumnInfo], bool] | None, got {use_columns:?}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very very cool! Looks great overall.
2 main points:
- add rustdoc comments on main structs, methods,...
- API improvement on
FastExcelSeries
Not blocking but would love it :)
} | ||
|
||
#[derive(Debug, Clone, PartialEq)] | ||
pub struct FastExcelColumn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add doc
} | ||
|
||
#[derive(Debug, Clone, PartialEq)] | ||
pub enum FastExcelSeries { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add doc
I would also impl len()
and is_empty
+ IntoIterator
for each variant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, I didn't implement those, because the length of a Null Series cannot be known. I added into_$type
methods to access the raw data, and the docs recommend to use FastExcelColumn
over FastExcelSeries
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds a Rust crate implementation for fastexcel, enabling usage from Rust code in addition to the existing Python interface. The Rust API provides access to Excel sheet and table data through column-based structures, with optional Polars DataFrame integration.
Key changes include:
- Rust API implementation with
FastExcelColumn
andFastExcelSeries
types - Optional Polars integration for DataFrame conversion
- Feature-based compilation to separate Python and Rust dependencies
- Restructured codebase to support both Python and Rust usage patterns
Reviewed Changes
Copilot reviewed 34 out of 59 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/lib.rs |
Exposes public Rust API and maintains Python module compatibility |
src/types/mod.rs |
Restructures type exports for both Python and Rust usage |
src/data/mod.rs |
Implements FastExcelColumn and FastExcelSeries for Rust API |
src/types/excelreader/mod.rs |
Refactors ExcelReader to support both Python and Rust usage |
tests/fastexcel.rs |
Adds comprehensive Rust integration tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Also readme needs an update |
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
Signed-off-by: Luka Peschke <[email protected]>
@PrettyWood A second review would be great, sync with In addition to branch update, I did the following:
|
This allows to use fastexcel from rust. It can be used to generate Fastexcel columns, which are simple wrappers around owned data.
An optional
polars
extra allows to convert anExcelSheet
or anExcelTable
to aDataFrame
.Different cargo features have been added to distinguish between Python and Rust builds, as we don't need
arrow
for rust usage (polars ships its own implem when enabled)See rust integration tests for usage examples