register casting rule draft implementation - #37
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a per-column casting/validation override API to the loader casting pipeline so callers can register column-specific rules (notably to support sa.Enum columns and “implied enum” String(1) columns) while preserving existing type-based casting behavior for all other columns.
Changes:
- Introduces
register_column_cast_rule()and a per-column rule registry that takes precedence over type-basedCAST_RULES. - Threads
table_name/column_namethrough the Pandas casting path so per-column rules can be applied duringcast_to_model. - Adds unit and E2E/Postgres coverage for enum casting and custom per-column scalar validation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/models.py | Adds test tables/enums to exercise real sa.Enum and implied-enum String(1) scenarios. |
| tests/loaders/test_pg_loader.py | Adds a real-Postgres round-trip test verifying enum casting output matches what native PG enum storage expects. |
| tests/loaders/test_loader_e2e.py | Adds E2E tests proving column-specific precedence and custom scalar behavior. |
| tests/loaders/test_casting.py | Adds focused unit tests for rule registration semantics and precedence. |
| src/orm_loader/loaders/loader_interface.py | Passes table/column identifiers into perform_cast during Pandas casting. |
| src/orm_loader/loaders/data/converters.py | Implements column-specific casting registry and hooks it into scalar/Arrow casting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/orm_loader/loaders/data/converters.py:92
- Same as the string branch above:
pa.nulls(len(arr), arrow_type)materializes an N-length array; using a typed null scalar lets Arrow broadcast efficiently (and avoids potential mismatches whenarris chunked).
return pc.if_else( # type: ignore
pc.is_nan(arr), # type: ignore
pa.nulls(len(arr), arrow_type),
arr,
)
src/orm_loader/loaders/data/converters.py:85
- Using
pa.nulls(len(arr), arrow_type)here allocates a full-length null array and can also be a poor fit whenarris apa.ChunkedArray(the Parquet loader passes chunked columns). Prefer a typed null scalar sopc.if_elsecan broadcast without materializing an N-length array and without Array/ChunkedArray shape mismatches.
This issue also appears on line 88 of the same file.
return pc.if_else( # type: ignore
pc.is_in(probe, value_set=_ARROW_NULL_STRINGS), # type: ignore
pa.nulls(len(arr), arrow_type),
arr,
)
src/orm_loader/loaders/data/converters.py:406
ParquetLoader.cast_to_model()passesarr = data[col_name], which is apa.ChunkedArrayfor typicalpa.Tablecolumns. The current type hints (arr: pa.Array/ return-> pa.Array) are misleading and don’t match actual call sites orpc.cast/pc.if_elsereturn types when chunked input is used.
def cast_arrow_column(arr: pa.Array, sa_col: ColumnElement[Any], stats: TableCastingStats | None = None) -> pa.Array:
src/orm_loader/loaders/data/converters.py:295
register_column_cast_rule()assumesenum_typeis anEnumsubclass. If a caller accidentally passes a non-enum (or an enum instance), the failure will be deferred to later casting and the error will be less clear. Consider validatingenum_typeat registration time and raising aTypeErrorwith a targeted message.
if enum_type is not None:
if scalar is not None:
raise ValueError("register_column_cast_rule requires exactly one of `scalar` or `enum_type`.")
resolved_scalar = _enum_member_scalar(enum_type)
elif scalar is not None:
tests/loaders/test_loader_e2e.py:203
- Fix typo in comment:
enum_type='sshould beenum_type's.
# expects the raw code itself ('S'), not enum_type='s default .name output
Summary
closes #36
Checklist
breaking,feature,fix,dependencies, orchore)uv run pytest -q)uv run ruff check .)