Improved read_excel(..., engine='calamine') performance by 10-20% in .xlsx - #197
Open
HasSak-47 wants to merge 8 commits into
Open
Improved read_excel(..., engine='calamine') performance by 10-20% in .xlsx #197HasSak-47 wants to merge 8 commits into
HasSak-47 wants to merge 8 commits into
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds CalamineSheet.to_python_pandas and convert_to_pandas_cell plus BigInt support and tests; exports nested Python lists with pandas-style coercions (integral floats → int/BigInt, specialized DateTime/DateTimeIso handling). ChangesPandas conversion
Sequence DiagramsequenceDiagram
participant Client
participant Sheet as CalamineSheet
participant Conv as convert_to_pandas_cell
participant Data as CellData
Client->>Sheet: to_python_pandas(skip_empty_area, nrows)
Sheet->>Sheet: compute range / apply nrows
loop per row
loop per cell
Sheet->>Data: read cell data
Sheet->>Conv: convert_to_pandas_cell(&Data)
alt Float exactly integral
Conv->>Conv: produce Int or BigInt
else DateTime / DateTimeIso
Conv->>Conv: choose Timedelta / Time / DateTime
Conv->>Conv: validate year range
alt invalid / parse fail
Conv->>Conv: fallback to Float or String
end
else Other
Conv->>Conv: default conversion
end
Conv-->>Sheet: CellValue
Sheet->>Sheet: append to Python row list
end
Sheet->>Sheet: append row to result list
end
Sheet-->>Client: nested PyList of converted rows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/types/cell.rs (1)
35-77: Significant duplication with existingFrom<&DT> for CellValue.
convert_to_pandas_cellre-implements most of theFrom<&DT>logic (lines 99–169) with two deliberate differences:
- Integer-valued floats are coerced to
Intinstead of left asFloat.- Integer-valued serial datetimes become
DateTime(at midnight) instead ofDate, andDateTimeIsodates become midnightDateTimeinstead ofDate.It might be worth refactoring
From<&DT>to take a smallPandasModeflag (or extract the shared temporal dispatch into a helper that both call sites use with differentDate/DateTimemapping closures), so the two conversion paths can't drift apart on unrelated fixes (e.g., year-range handling, duration detection).Also, nit on Line 40:
f.is_finite() && !f.is_nan()—is_finite()already implies!is_nan(), so the!f.is_nan()check is redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/cell.rs` around lines 35 - 77, convert_to_pandas_cell duplicates the temporal conversion logic from the From<&DT> for CellValue implementation and uses a slightly different mapping (coerce integer-valued floats to Int and map integer serial datetimes to midnight DateTime), which risks drift; refactor by extracting the shared temporal dispatch into a single helper (e.g., a function used by both convert_to_pandas_cell and the From<&DT> impl) that accepts small behavior flags or mapping closures (Pandas-mode) for the final Date/DateTime/Time/Timedelta decisions and reuse check_year_range and the Data::DateTime / Data::DateTimeIso branches to avoid duplication, and remove the redundant !f.is_nan() check from the Data::Float arm (keep f.is_finite() only).tests/test_pandas_bypass.py (1)
28-80: Rename tests and de-duplicate the four near-identical cases.The four
test_old_pandas_*functions are 95% identical and theold_prefix is misleading — they test the newto_python_pandaspath against a reference conversion. Consider parametrizing:`@pytest.mark.parametrize`("ext", ["xlsx", "xls", "xlsb", "ods"]) deftest_to_python_pandas_matches_reference(ext): sheet_names= ["Sheet1", "Sheet2", "Merged Cells"] wb=CalamineWorkbook.from_object(PATH/f"base.{ext}") forsheet_nameinsheet_names: sheet=wb.get_sheet_by_name(sheet_name) expected= [[_reference_convert_cell(y) foryinx] forxinsheet.to_python()] assertsheet.to_python_pandas() ==expectedAlso, the
__(dunder) prefix on__old_convert_cellat module scope is unusual; a single leading underscore is the idiomatic "module-private" marker in Python.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pandas_bypass.py` around lines 28 - 80, Replace the four near-duplicate test functions (test_old_pandas_xlsx, test_old_pandas_xls, test_old_pandas_xlsb, test_old_pandas_ods) with a single parametrized test that iterates over extensions and sheet names, calls CalamineWorkbook.from_object(PATH / f"base.{ext}"), compares sheet.to_python_pandas() to the reference conversion of sheet.to_python(), and removes the pprint calls; also rename the module helper __old_convert_cell to _reference_convert_cell and update its usages to avoid using a double-underscore module-level name. Ensure the new test function name reflects testing to_python_pandas (e.g., test_to_python_pandas_matches_reference) and keep references to the to_python_pandas and CalamineWorkbook.from_object symbols when updating assertions.src/types/sheet.rs (1)
191-222: Extract shared implementation betweento_pythonandto_python_pandas.The body of
to_python_pandasis a near-verbatim copy ofto_python(lines 224–255); the only difference is the per-cell converter in thePyList::newcall. This duplicates thenrows/skip_empty_arearange-slicing logic, so any future fix to that logic (e.g., thenrows - 1underflow whennrows == 0) must be applied in two places.♻️ Suggested refactor
fnbuild_rows<'py,F>(slf:&PyRef<'py,Self>,skip_empty_area:bool,nrows:Option<u32>,convert:F,) -> PyResult<Bound<'py,PyList>>whereF:Fn(&Data) -> CellValue,{let nrows = nrows.unwrap_or_else(|| slf.range.end().map_or(0, |end| end.0 + 1));let range = if skip_empty_area || Some((0,0)) == slf.range.start(){Arc::clone(&slf.range)}elseifletSome(end) = slf.range.end(){Arc::new(slf.range.range((0,0),(if nrows > end.0{ end.0}else{ nrows - 1}, end.1),))}else{Arc::clone(&slf.range)};let py_list = PyList::empty(slf.py());for row in range.rows().take(nrows asusize){ py_list.append(PyList::new(slf.py(), row.iter().map(&convert))?)?;}Ok(py_list)}Then
to_pythoncalls it with<&Data as Into<CellValue>>::intoandto_python_pandaswithconvert_to_pandas_cell.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/sheet.rs` around lines 191 - 222, to_python_pandas duplicates the range-slicing and nrows logic from to_python (risking bugs like the nrows - 1 underflow), so extract the shared logic into a new helper fn build_rows<'py, F>(slf: &PyRef<'py, Self>, skip_empty_area: bool, nrows: Option<u32>, convert: F) -> PyResult<Bound<'py, PyList>>> where F: Fn(&Data) -> CellValue (or appropriate signature) that computes nrows with unwrap_or_else(slf.range.end().map_or(0, |end| end.0 + 1)), computes range using the same conditions but avoid nrows - 1 underflow (use nrows.saturating_sub(1) or conditional logic), builds the PyList by iterating range.rows().take(nrows as usize) and applying convert, then change to_python to call build_rows with <&Data as Into<CellValue>>::into and change to_python_pandas to call build_rows with convert_to_pandas_cell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types/cell.rs`:
- Around line 40-41: The float-to-int conversion in the Data::Float match
currently uses a round-trip cast ((*f as i64) as f64 == *f) which falsely
accepts saturated values near i64::MAX; update the Data::Float handling to
require f.is_finite(), *f >= i64::MIN as f64, *f < i64::MAX as f64, and
f.fract() == 0.0 before casting to Data::Int, otherwise fall back to returning
the original data.into(); apply this change in src/types/cell.rs where the
Data::Float -> Data::Int coercion occurs to prevent out-of-range floats from
being coerced.
In `@tests/test_pandas_bypass.py`:
- Around line 9-25: Extend tests in tests/test_pandas_bypass.py to explicitly
cover pandas-specific coercions missing from __old_convert_cell by adding small,
targeted unit cases that compare behavior of the pandas path
(convert_to_pandas_cell) against expected outputs: (1) float x.0 → int coercion,
(2) non-finite floats (NaN/Inf) remain float, (3) Data::DateTime routing to
Timedelta vs Time vs DateTime using is_duration() and v < 1.0 scenarios, (4)
Data::DateTime fallback to float when year is outside [1,9999] or conversion
fails, and (5) Data::DateTimeIso parsing that uses 'T' / ':' to pick datetime vs
returning the raw ISO string when parsing fails; use small constructed
fixtures/inputs for each case and assert convert_to_pandas_cell returns the
expected type/value to prevent regressions.
- Around line 64-80: The test test_old_pandas_ods contains debug artifacts:
remove the mid-file import "from pprint import pprint" (either drop it or move
it up with other imports) and delete the three debugging calls
pprint(sheet.to_python()), pprint(old_data), and pprint(new_data) so the test
only builds old_data via __old_convert_cell over sheet.to_python() and compares
it to sheet.to_python_pandas() against the CalamineWorkbook.from_object(PATH /
"base.ods") fixture.
---
Nitpick comments:
In `@src/types/cell.rs`:
- Around line 35-77: convert_to_pandas_cell duplicates the temporal conversion
logic from the From<&DT> for CellValue implementation and uses a slightly
different mapping (coerce integer-valued floats to Int and map integer serial
datetimes to midnight DateTime), which risks drift; refactor by extracting the
shared temporal dispatch into a single helper (e.g., a function used by both
convert_to_pandas_cell and the From<&DT> impl) that accepts small behavior flags
or mapping closures (Pandas-mode) for the final Date/DateTime/Time/Timedelta
decisions and reuse check_year_range and the Data::DateTime / Data::DateTimeIso
branches to avoid duplication, and remove the redundant !f.is_nan() check from
the Data::Float arm (keep f.is_finite() only).
In `@src/types/sheet.rs`:
- Around line 191-222: to_python_pandas duplicates the range-slicing and nrows
logic from to_python (risking bugs like the nrows - 1 underflow), so extract the
shared logic into a new helper fn build_rows<'py, F>(slf: &PyRef<'py, Self>,
skip_empty_area: bool, nrows: Option<u32>, convert: F) -> PyResult<Bound<'py,
PyList>>> where F: Fn(&Data) -> CellValue (or appropriate signature) that
computes nrows with unwrap_or_else(slf.range.end().map_or(0, |end| end.0 + 1)),
computes range using the same conditions but avoid nrows - 1 underflow (use
nrows.saturating_sub(1) or conditional logic), builds the PyList by iterating
range.rows().take(nrows as usize) and applying convert, then change to_python to
call build_rows with <&Data as Into<CellValue>>::into and change
to_python_pandas to call build_rows with convert_to_pandas_cell.
In `@tests/test_pandas_bypass.py`:
- Around line 28-80: Replace the four near-duplicate test functions
(test_old_pandas_xlsx, test_old_pandas_xls, test_old_pandas_xlsb,
test_old_pandas_ods) with a single parametrized test that iterates over
extensions and sheet names, calls CalamineWorkbook.from_object(PATH /
f"base.{ext}"), compares sheet.to_python_pandas() to the reference conversion of
sheet.to_python(), and removes the pprint calls; also rename the module helper
__old_convert_cell to _reference_convert_cell and update its usages to avoid
using a double-underscore module-level name. Ensure the new test function name
reflects testing to_python_pandas (e.g.,
test_to_python_pandas_matches_reference) and keep references to the
to_python_pandas and CalamineWorkbook.from_object symbols when updating
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96e8bec4-fa07-4561-9da2-687b56b248c3
📒 Files selected for processing (4)
python/python_calamine/_python_calamine.pyisrc/types/cell.rssrc/types/sheet.rstests/test_pandas_bypass.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
HasSak-47
commented
May 24, 2026
Author
@dimastbk Hi is there any plan to add this feat? or should I close this pr? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This adds an opt-in
Worksheet.to_python_pandas()method that returns data using the coercions pandas currently applies after reading frompython-calamine.This PR adds CalamineSheet.to_python_pandas(), which returns data already normalized to match pandas’ expectations. This avoids the extra post-processing pass and reduces overhead in the read_excel(..., engine="calamine") path.
The existing
to_python()behavior is unchanged.Performance
Benchmark comparing identical environments:
Median speedup: 1.20x
Median runtime reduction: −16.7%
95% CI: [1.09x, 1.30x]
Faster in 17 / 21 matched .xlsx files
Improvements are more pronounced on larger files (>10MB), consistent with reduced per-cell overhead.
Summary by CodeRabbit