refactor(relational): labels and the result are modules, not regions of the executor - #368
Conversation
…of the executor `executor.py` was 1070 lines holding four separable jobs and one piece of public API. Two of them come out; the file is 709 lines and the split is along seams the architecture doc already described in prose but the code did not have a unit for. **`relational/labels.py`.** ARCHITECTURE gives labelling its own paragraph — "labels are the one place order is load-bearing", three routes to one number that "must agree integer for integer, because a label *is* a solver index" — but the code was `_label_frame` / `_factored` / `_row_major` sitting among twenty other methods and reaching through `self` for what they needed. A `Labeller` states its inputs instead: the query, the dimension cardinalities, and the program. Nothing else about a build can now move a solver index, which is the claim the paragraph was already making. **`relational/result.py`.** `Result` is what `lps.solve` returns — the one object in this subpackage a reader meets without going looking — and it lived above `PolarsExecutor` in a module named for an internal build phase. It also does the opposite job: the executor *builds* a model, `Result` *reads* one back through label joins. `api.py` and `relational/__init__.py` now import it from where it is. Pure code motion: no logic changed, and `tests/test_architecture.py` caught the one thing that did need writing — a new module with no home in the module map — which is the check working as intended. Considered and not done: extracting source binding (`_create_param_frame`, `_explicit_dim_frame`, the two dim-frame passes, ~180 lines). It is a coherent unit, but the three passes are ordered and interdependent — sourced dims first so a parameter's labels can be checked as it binds, then parameters, then the dims derived from them — and the ordering argument lives in `build()`. Splitting it would either move that argument away from the sequence it explains or hand four pieces of state back through a carrier built only to be unpacked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
1 participant
executor.pywas 1070 lines holding four separable jobs plus one piece ofpublic API. Two come out. The file is now 709 lines, and both splits fall
along seams
docs/ARCHITECTURE.mdalready described in prose but the code hadno unit for.
Pure code motion — no logic changed. 700 passing, ruff clean, pyrefly 0 errors.
executor.pylabels.pyresult.pyrelational/labels.pyARCHITECTURE gives labelling its own paragraph — "labels are the one place
order is load-bearing", three routes to one number that "must agree integer
for integer, because a label is a solver index". But the code was
_label_frame/_factored/_row_majorsitting among twenty other methods,reaching through
selffor the three things they needed alongside the ten theydidn't.
A
Labellerstates those inputs: the query, the dimension cardinalities, theprogram. Nothing else about a build can now move a solver index — which is the
claim that paragraph was already making, now checkable by reading a
constructor.
relational/result.pyResultis whatlps.solvehands back: the one object in this subpackage areader meets without going looking for it. It lived above
PolarsExecutorin amodule named for an internal build phase. It also does the opposite job — the
executor builds a model,
Resultreads one back through label joins — so thetwo being one file was working against both names.
api.pyandrelational/__init__.pynow import it from where it is.Considered and not done
Extracting source binding (
_create_param_frame,_explicit_dim_frame,the two dim-frame passes — about 180 lines). It is a coherent unit and it is
the largest remaining region, but the three passes are ordered and
interdependent: sourced dims first, so a parameter's labels can be checked in
the pass that binds it rather than a second one over the same rows; then the
parameters; then the dims derived from those. That argument lives in
build(), next to the sequence it explains. Splitting it would either strandthe argument away from the code it justifies or hand four pieces of state back
through a carrier object built only to be unpacked immediately.
Note
tests/test_architecture.py::test_every_module_is_documented_somewherefailedon the first run — a new module with no row in the module map. That is the
check doing its job;
docs/ARCHITECTURE.mdis updated in this PR, includingthe four prose references to
_label_framethat would otherwise have gonestale.
🤖 Generated with Claude Code