Repository navigation
fix(execution): audit native pandas extension dtype changes - #524
JohnnyWilson16 merged 6 commits into
Conversation
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a90017a17d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| original_dtype = source_dtypes.iloc[position] | ||
| if not is_extension_array_dtype(original_dtype): | ||
| continue | ||
| output_label = rename_map.get(native_name, native_name) |
There was a problem hiding this comment.
When DuckDB's case-insensitive registration suffix collides with a later normalized name—for example input headers A, a, and A 1 with an extension dtype on A 1—registration produces A, a_1, and A 1, while _project_clean maps both of the latter outputs to a_1 and DuckDB disambiguates the projected result again. This lookup still resolves both source positions to the pre-projection a_1, so it reads the second column's dtype (or gets a duplicate-column DataFrame and raises on .dtype) and misses or mislabels the event for the final suffixed column; derive the audit mapping from the final output names/positions instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cb8b2c8. The audit maps retained source ordinals to actual final output positions and reads dtypes through iloc, so second-round SQL suffixes and duplicate pandas labels do not redirect the comparison. All three new review cases fail before this correction; all 97 focused checks pass across pandas 1.5/2.3 and Polars 1.36/1.42/2.0. The full suite has 7,656 passes, with only the same five known Windows failures. I also reproduced and reported the separate underlying nonempty-column loss as #527.
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
JohnnyWilson16
left a comment
JohnnyWilson16
left a comment
There was a problem hiding this comment.
LGTM! Delivers non-destructive extension dtype auditing for #465 under step="output_dtype". Positional column slicing safely handles duplicate labels, categorical levels are kept private, and native handles remain unmaterialized. Ready to merge
…n-dtype-audit-465
…n-dtype-audit-465
…n-dtype-audit-465
2 participants
Native Polars/DuckDB runs can return pandas Int64 as int64, Float64 as float64, or string as object without recording the schema change. Record a per-column output_dtype backend difference after pandas conversion, including the backend, actual output column name, and original/result dtype names or storage. Track retained source positions through plan renames and DuckDB registration/projection suffixes, compare by position even with duplicate output labels, skip dropped columns, and avoid exposing category domain values in diagnostics.
This takes the audit option requested in #465. It preserves the native results and requested type-changing operations rather than forcing casts, retains the existing exact nullable-integer conversion, and does not materialize native handles or non-pandas outputs for the check. Document the behavior in the backend guide and changelog.
Validation: 40 new cases pass. The original 37 cases have 23 failures against the unchanged implementation and 14 positive controls; all three added review cases fail before the final-position correction. All 97 focused audit/parity/ingestion checks pass on pandas 1.5 with Polars 1.36, pandas 2.3 with locked Polars 1.42, and a separate floating Polars 2.0 / NumPy 2.5 environment. They cover exact wide integers, nullable text/boolean/float, renamed and implicitly registered headers, dropped columns, explicit fractional imputation, empty schemas, JSON serialization, and native handle preservation. The full fast suite has 7,656 passes, 49 skips, and 94.99% coverage; its five failures match the reproduced Windows baseline (performance-runner timeout, console encoding, generated-code filename escaping, CSV newline round trip, and the zero-budget boundary fixed in #520). Ruff, mypy across 205 sources, strict MkDocs, and diff checks pass.
Closes #465