[TexasCoding]

Closes #264

Part of #273 (post-v2.4 audit), Wave 2.

[claude]

Code Review — perf: build Page DataFrames column-oriented (#264)

Overview

This PR replaces the row-oriented model_dump() loop in Page.to_dataframe() / Page.to_polars() with a column-oriented _columns() helper that uses getattr per field, calling model_dump() only on cells that contain a nested BaseModel. The goal is to reduce redundant per-row serialization overhead for typical large pages. The approach is sound and the test coverage is solid. A few issues worth addressing before merge:

---

Bug: isinstance(col[0], BaseModel) fails for nullable nested fields

kalshi/models/common.py, _columns(), line 85–86

if col and isinstance(col[0], BaseModel):
    col = [v.model_dump(mode="python") if isinstance(v, BaseModel) else v for v in col]

The check samples only the first row to decide whether to dump the column. If the field is Optional[_Inner] and the first item happens to be None, the check short-circuits and the rest of the rows (which may hold live BaseModel instances) are left un-dumped in the column. This would cause pandas/polars to receive a mix of None and Pydantic model objects and likely raise a TypeError or silently store opaque objects.

Fix: scan all elements instead of just col[0], or change the heuristic to check whether any value in the column is a BaseModel:

if any(isinstance(v, BaseModel) for v in col):
    col = [v.model_dump(mode="python") if isinstance(v, BaseModel) else v for v in col]

There's no test covering a page where the first item has None for a nullable nested field but later items have an actual sub-model. Adding one would pin this.

---

Potential silent divergence: model_fields bypasses Pydantic serialization logic

_columns(), line 81

fields = type(self.items[0]).model_fields.keys()

model_fields is the declared-field registry. The old model_dump() path honored:

• exclude=True on individual fields
• model_computed_fields (computed properties)
• serialization aliases

The new getattr path bypasses all of those. For current SDK models this is likely a non-issue (no excluded fields, no computed fields), but it's a subtle semantic gap. If a future model uses Field(..., exclude=True) it will silently appear in the DataFrame output when it shouldn't.

Either document this explicitly or use item.model_dump(mode="python").keys() from the first item to derive the field list (still O(1) calls, still avoids the N×fields walk).

---

Style: multi-line docstring on a private helper

_columns() docstring

CLAUDE.md says "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The _columns docstring is 5 lines. Trim it to the one-liner, or drop it entirely since the function name is self-explanatory and the issue references belong in the commit message / PR body, not in the source.

---

Minor: cosmetic formatting changes are off-scope

The diff reformats several function signatures (collapses two-line def into one line) in the test file. These aren't related to the perf change and violate the Surgical Changes principle. No correctness concern, but they add noise to the diff and would need to be reverted if this file is under active review elsewhere.

---

Test coverage — good, with two gaps

The new TestPageToDataframeColumnOriented and TestPageToPolarsColumnOriented classes are well-structured and pin the Decimal contract at scale. Two gaps to fill:

1. Nullable nested field, first item is None — needed to catch the bug described above.
2. Column set matches model_dump() output — a test that calls both _columns() and model_dump() on the same item and asserts the key sets are equal would guard against the silent divergence risk.

---

Summary

Performance change	Correct direction; getattr-based column build is faster for flat models
Core bug	col[0] nullable check can silently mishandle Optional[BaseModel] fields
Semantic risk	model_fields vs model_dump() field set divergence
Tests	Solid at scale, two edge cases missing
Style	Docstring length; cosmetic test reformats

The perf improvement is real and the main logic is clean — the nullable-nested-field edge case is the one thing that needs fixing before this merges.