-
Notifications
You must be signed in to change notification settings - Fork 18
fix: use absolute column index for DTypeMap and support deferred selections for tables with use_columns #440
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
base: main
Are you sure you want to change the base?
Conversation
…ctions for tables with use_columns 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.
Pull request overview
This PR fixes column selection and dtype handling for Excel tables and sheets with offsets, ensuring that absolute column indices are used consistently throughout the codebase. The main changes include support for deferred selections (range-based column selections like "A:B" or "D:") on tables and using absolute indices for dtype lookups.
Key changes:
- Fixed dtype lookups to use absolute column indices instead of relative indices
- Added support for deferred column selections (range-based) on Excel tables
- Improved test coverage for column selection and dtype handling with offset tables
- Fixed flaky test by using pytest's caplog instead of mocker for logging assertions
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/column_selection.rs |
Added comprehensive Rust tests for column selection with offset tables and dtypes |
src/types/exceltable/mod.rs |
Added helper function and deferred selection support for tables |
src/types/excelsheet/mod.rs |
Extracted deferred selection resolution logic into reusable function |
src/types/excelsheet/column_info/mod.rs |
Fixed dtype and column name lookups to use absolute indices |
python/tests/test_dtypes.py |
Fixed flaky logger test using caplog instead of mocker |
python/tests/test_column_selection.py |
Updated dtype expectations and added Python tests for column selection with dtypes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IdxOrName::Name(name) => table_columns | ||
| .iter() | ||
| .enumerate() | ||
| .find_map(|(idx, col_name)| (col_name.as_str() == name.as_str()).then_some(idx)) |
Copilot
AI
Dec 21, 2025
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.
The function returns a table-relative index when looking up by name, but later compares it with absolute indices. When a column name is found in table_columns at position idx, this idx is relative to the table (0-based), not absolute. However, on line 70, the code checks if selected_column_indices contains (idx + column_offset), which is an absolute index. This mixing of relative and absolute indices will cause columns looked up by name to be incorrectly filtered out. The name lookup should return (idx + column_offset) to convert to an absolute index, matching the behavior of IdxOrName::Idx.
| .find_map(|(idx, col_name)| (col_name.as_str() == name.as_str()).then_some(idx)) | |
| .find_map(|(idx, col_name)| { | |
| (col_name.as_str() == name.as_str()).then_some(idx + column_offset) | |
| }) |
Follow-up work on #438 , #437 and #434 . Work items:
use_columnscan be used with table in excel range form"A:B"dtypesparameter are also absolutetest_fallback_infer_dtypeslogged a message