Skip to content

perf(engine): cols.vtype is an Enum, not a word repeated per column - #211

Merged
FBumann merged 2 commits into
polars-enginefrom
perf/carry-over-vtype-enum
Jul 28, 2026
Merged

perf(engine): cols.vtype is an Enum, not a word repeated per column#211
FBumann merged 2 commits into
polars-enginefrom
perf/carry-over-vtype-enum

Conversation

@FBumann

@FBumann FBumann commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Carries #174 onto polars-engine. That PR measured the right thing against the wrong engine — it is CREATE TYPE ... AS ENUM and DESCRIBE cols against duckdb, which #189 deletes, so it cannot merge as written. The reasoning survives the engine; the SQL does not.

What it costs today

vtype holds one word per column, and the same three words for the whole model. As a string, polars stores that word once per row:

at 9.8M columns
vtype as String 0.098 GB
vtype as Enum 0.010 GB
cols frame 0.333 → 0.245 GB
four model frames 0.77 → 0.68 GB

Measured on transport/l, which is the case where the cols frame is largest.

The members are the contract

The storage is the reason to do it; the vocabulary is the reason to test it. An Enum rejects a value outside its categories, so a fourth entry added to plan.VariableType and not reaching this column now fails where the column is built — rather than downstream in whichever sink first compares against a name it does not know (lp_file filters on it, highs tests it for integrality).

#174's test comes with it, rewritten against the frame instead of DESCRIBE cols. I checked it bites: reverting the dtype to String fails both tests in the file.

One wrinkle

_DTYPES maps column names to dtype names rather than dtypes, because the executor imports polars lazily and a module-level pl.Enum(...) would force the import at module load. An Enum has to be constructed, so this adds _dtype() to resolve them rather than a fourth string entry.

399 passed / 1 xfailed, ruff and pyrefly clean.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9187d5f6-13d6-4c32-aa08-cf34f40b0bd8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/carry-over-vtype-enum

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@FBumann

FBumann commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Why can't we change the import to eager? polars is a hard dep there...

We can — you're right that availability isn't the constraint, and I overstated the case in the PR body. polars>=1.30 is in [project] dependencies, not an extra, so it is always there. Measured, so the trade is explicit rather than a convention nobody re-examined:

import farkas today 84 ms
polars on its own 49 ms
polars in sys.modules after import farkas False
polars in sys.modules after fk.check(...) False

farkas/__init__ already imports relational.executor — the module costs 0 ms to import precisely because its polars imports are in functions. Hoist them and every import farkas pays the 49 ms, taking it to ~133 ms.

So the only thing the convention buys is fk.check. Validation parses YAML and reads no data, and today it never touches polars. Everything else — build, solve, write — pays the import anyway, just later.

Worth being clear that the hard rule doesn't apply here either: ARCHITECTURE.md rule 0 and test_lazy_intra_package_imports_are_all_declared only govern farkas.* imports (node.module.startswith('farkas')). The 48 in-function import polars as pl sites across 6 modules are outside it — so they are a convention, not a constraint, and nothing fails if they move.

If you want it eager I'll do it as its own change rather than inside this one: it deletes _dtype() here (the Enum goes straight into _DTYPES as a dtype rather than a name), and touches compiler.py, frames.py and the three sinks too. Say the word and I'll take it, and this PR gets simpler as a result.

Carried over from #174, which measured this on duckdb and cannot merge — that
PR is `CREATE TYPE ... AS ENUM` against an engine #189 deletes. The reasoning
survives the engine; the SQL does not.

`vtype` holds one word per column and the same three words for the whole
model, so as a string it stores that word once per row. At 9.8M columns it is
0.098 GB of the `cols` frame's 0.333, against 0.010 as an Enum — the frame
goes 0.333 -> 0.245 GB, and the four model frames together 0.77 -> 0.68.

The members are the part worth testing. An Enum rejects a value outside it, so
a fourth variable type added to `plan.VariableType` and not reaching the
column now fails where the column is built, rather than in whichever sink
first compares against a name it does not know. #174's test is carried over
with it, rewritten against the frame instead of `DESCRIBE cols`; both tests in
the file fail if the dtype goes back to String.

`_DTYPES` maps names rather than dtypes because polars is imported lazily, and
an Enum has to be constructed — hence `_dtype()` rather than a fourth entry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@FBumann
FBumann force-pushed the perf/carry-over-vtype-enum branch from cb7f9e0 to 528d679 Compare July 28, 2026 07:41
…ncy it is

48 in-function `import polars as pl` across six modules, none of them
load-bearing. polars is in `[project] dependencies`, not an extra, so the
import could never fail; and ARCHITECTURE.md rule 0 governs `farkas.*` imports
only — `test_lazy_intra_package_imports_are_all_declared` matches on
`node.module.startswith('farkas')` — so these were a convention rather than a
constraint, and nothing enforced them.

What the convention bought, measured: `import farkas` goes 87 ms -> 119 ms,
because `farkas/__init__` already imports the executor and the lazy imports
were the only reason that cost 0. The +32 ms is less than polars' 49 ms
standalone, since numpy and pyarrow are shared.

Who pays: `fk.check`, which parses YAML, reads no data, and never touched
polars before. `build`, `solve` and `write` all paid it anyway, one function
call later.

`sinks/tables.py` keeps its import in TYPE_CHECKING — it is four dataclass
annotations and no runtime use, which is the one place the block is the honest
answer rather than a habit.

Drops `_dtype()`, added one commit ago only because an Enum has to be
constructed and polars was not in scope at module level. `_DTYPES` now holds
dtypes rather than names for them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@FBumann FBumann added the engine:polars Specific to the polars rewrite (#189) — invalidated if duckdb stays label Jul 28, 2026
@FBumann

FBumann commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto polars-engine after #212 landed — that is what the conflict was.

One thing to flag: I clobbered your merge of #213 and have restored it. You merged #213 into this branch at 07:40 (cb7f9e0). My rebase started from a stale local ref, so --force-with-lease compared against what I had last fetched rather than the true remote, passed, and dropped that merge commit.

This branch now carries both commits — the vtype Enum and the eager polars import — replayed on top of #212 rather than merged, so it is linear and conflict-free. Same content your merge produced; aa99be6 has 0 lazy polars imports, the Enum, and #212's factored labels from the base.

400 passed / 1 xfailed, ruff and pyrefly clean.

@FBumann
FBumann merged commit b4b9e5c into polars-engine Jul 28, 2026
2 checks passed
FBumann added a commit that referenced this pull request Jul 28, 2026
main gained the three duckdb perf PRs (#152, #174, #178), two language-level
fixes (#201, #223) and the bench harness from #205. Resolved by lane:

**Engine files take the polars side.** #152, #174 and #178 are `ROW_NUMBER()
OVER`, `CREATE TYPE ... AS ENUM` and `CREATE TABLE surv_*` against the engine
this branch deletes. Their *ideas* are already here in polars form — #152's as
`_positional`, #174's as #211, #178's as #212 — so taking main's side would
have re-added SQL against a connection that no longer exists.

**Language and validation take main's.** #223's `lowering.py`,
`validation.py` and `linopy/builder.py` changes merged clean and are engine
independent; its executor half and #201's are already here (as #225 and
`_check_one_row_per_coordinate`). `test_objective.py` and `test_validation.py`
arrived whole and pass on this lane unmodified — 404 tests to 414.

**bench takes ours**, which carries #205's two ideas ported rather than
merged (#215): the duckdb commit also brings `--memory-limits`, `--chunk-rows`
and `workdir_bytes`, the budget apparatus this branch removed.

Two tests were combined rather than picked. `test_group_sum`'s docstring says
both true things now — the divergence between lanes, and the aggregate-skip
premise that rests on it. `test_milp` keeps the polars Enum test and gains
main's ragged-chunk test, which is API-level and applies to this sink too.

Main's duckdb label tests are dropped, not ported: they drive `DuckdbExecutor`
and `ex._con` directly. The properties they pin are covered here by
`test_a_mask_that_removes_nothing_labels_exactly_like_no_mask` and
`test_a_factored_mask_labels_exactly_like_the_counted_path`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann added a commit that referenced this pull request Jul 28, 2026
Full ladder after merging main: five cases, two sinks, both arms, best of
three, parity gate green on all five. 288 records, no failures.

**Every ratio is unchanged within noise, and that is the expected result.**
main's three duckdb perf PRs cannot move this comparison: neither arm here is
duckdb. The farkas arm is the polars engine and the linopy arm is
`farkas.linopy.build`, so the work in #152, #174 and #178 has nothing to act
on — their ideas already arrived on this branch as `_positional`, #211 and
#212, and it is those that the numbers already reflected.

`transport` reads 1.59x / 0.81x where it read 1.62x / 0.78x, `sector` 0.31x
where it read 0.32x, and so on. The tables and every figure quoted in the
prose are refreshed off the new file so the doc and its provenance agree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann added a commit that referenced this pull request Jul 28, 2026
…sult (#231)

* bench: a case where I/O is not noise, and a sink axis to measure it on (#205)

Two changes to the harness, both about measuring what users actually do.

**`profiled`, the case whose input is the same size as its model.** Every
existing case builds its coordinate product from parameters far smaller than
the product they explode into — 2-15% of it — so reading the parquet is 2-9%
of the build and the read path is lost in the margin. `profiled` is `nodal`'s
cardinalities with the mask removed and `availability` dense over
`(snapshot, node, tech)`: at the `l` rung, a 12M-row table against a 12M
coordinate product. That makes the read path, the join, and the
in-memory-vs-parquet question measurable rather than noise.

It is also the shape the eager lane handles best — a parameter already dense
over the variable product is the array xarray wants, no broadcast and no
alignment — while we join a full-size frame against a full-size coordinate
product. `nodal` is the mirror image and our clearest win. Carrying both is
what keeps the ladder from holding only shapes that suit one engine. Closes #202.

**Both sinks, not just the LP file.** `--sinks lp highs`, defaulting to both.
The LP file is the artifact fewest callers want, and it is not the same
comparison: at `profiled/m` the peak ratio is 0.60x through the LP writer and
0.94x through the handoff, because HiGHS's own dense model is resident in both
arms and swamps the difference between them.

The `highs` sink stops at the handoff — `run()` is never called. The simplex is
the same work whoever filled the model, so including it would swamp the phase
this harness exists to measure. `solve_direct` is split into `build_highs` +
the solve so there is a seam to stop at; linopy's `Model.to_highspy()` is the
same seam on the other side, which is what makes the two arms comparable.
A rung the sink is capped out of is written to the JSONL as a `skipped` record
and footnoted in the report, so a coverage hole cannot read as a result.

Measured, `profiled` at the `l` rung (12M variables): LP file, farkas 8.42 s /
1.49 GB against linopy 2.44 s / 3.26 GB; HiGHS handoff, farkas 7.24 s / 2.60 GB
against linopy 2.93 s / 3.23 GB. The parity gate passes on the new case.

No published numbers change here: `bench/results/latest.jsonl` is left as it
was, since re-running the ladder is its own act.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: refuse a parameter source that carries a coordinate twice (#201)

A parameter is a function of its dims, so two rows for one coordinate has no
defined answer. The eager lane already refuses such a source — `xarray` will
not lay out a duplicated index — while the relational lane quietly resolved it
into a sum. Two lanes that accept the same language were accepting different
data, and the one that accepted more gave an answer nobody asked for.

Now it raises a `DataError` naming the parameter and up to three offending
coordinates. One `GROUP BY … HAVING count(*) > 1` per parameter, over a source
orders of magnitude smaller than the model built from it.

`test_a_coordinate_must_be_single_valued` had to be narrowed: it doubled the
whole generator frame, which feeds both the coordinate index *and* two
parameters, so the new check now fires first and correctly. It doubles only the
index; the parameter case is its own test beside it.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(main): release 0.0.0-alpha.27 (#210)

Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com>

* fix: three ways an objective or a scalar parameter answered quietly (#223)

* fix: an objective sums the terms it names, once each

Two ways an objective quietly answered a question nobody asked.

`x * a + y * b`, with `x, a` on `i` and `y, b` on `j`, is `|i| + |j|` summands.
The relational lane knew that — an expression there is a set of term
fragments, each keeping its own dims until the objective sums it. The eager
lane added the two operands first, which is linopy's `+`, which broadcasts:
every term counted once per coordinate of its sibling, `|i| * |j|` summands,
8.8x apart on the mixed-density case that found it (#197). Hard rule 3 says
the lanes mean the same thing, and here the relational one was right.

So the eager lane distributes the sum over addition and sums each product on
its own. The distribution has to survive an operator applied to the group —
`(x * a + y * b) * c` was wrong by the same factor — so `*`, `/` and unary
minus are walked too, and everything else stays one opaque term.

Separately, `objectives.<name>.equations` is a list of which only the first
entry was ever read. It reads as "several terms in one objective", because
that is what the key means under a constraint, and the answer was wrong by
whatever the ignored entries were worth with nothing to see (#198). It is now
a load error naming the rewrite, spelled from what was written.

SPEC said "only equations[0] is used" in §2 and "the last defined wins" for
multi-objective in §11 — the second was already false, since a second
objective has been a load error for a while.

Closes #197
Closes #198

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: a dimensionless parameter is one row, and now has to be

`_check_one_row_per_coordinate` returned early when a parameter had no dims,
on the reading that there was no coordinate to group by. But a parameter with
no dims has exactly one coordinate — the empty one — so the rule it was
skipping is the rule that matters most there.

The join that broadcasts a dimensionless parameter is `ON TRUE`, which is
correct broadcast semantics for one row and a silent row multiplication for
two: duplicate `cols` entries for one variable in a bound, duplicate mask rows
in a where. Both built and solved without a word. A user who gets here has
usually passed a column they thought was indexed, so the message says what a
dimensionless parameter means rather than only what is wrong with the source.

Zero rows is caught by the same count, and was the other half of the hole.

One `count(*)` per scalar parameter, at metadata level.

Closes #166

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(main): release 0.0.0-alpha.28 (#226)

Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com>

* perf: compute variable labels arithmetically when the mask factors (#178)

The label frame was the single most expensive statement in build, and the cost
was the sort inside

    ROW_NUMBER() OVER (ORDER BY t_snapshot.ord, t_generator.ord)

When the mask does not read the leading dim, that sort is avoidable without
changing a single label. `where: "p_max > 0"` depends only on generator, so
the surviving trailing coordinates are identical for every snapshot: rank them
once over a table of size prod(rest), and the label is
`(lead.ord - lo) * width + rank` — no window function, no per-chunk sort. The
general path stays for masks that do read the leading dim.

Measured with bench/run.py, best of the repeats, on this machine:

  case/size    build before  build after          vs linopy
  dispatch/m       1.27s        0.88s        2.20x -> 1.67x
  dispatch/l       8.66s        5.01s        1.35x -> 0.90x
  transport/m      1.56s        1.10s        2.03x -> 1.65x
  transport/l     10.67s        7.27s        1.65x -> 1.31x

dispatch/l crosses over: farkas is now faster than linopy end to end at 10M
variables. The absolute ratios are machine-specific and differ from the
committed macOS numbers, so the delta is the reliable part; docs/benchmarks.md
is left for a run on the machine its ladder was measured on.

Labels are unchanged, and that is checked rather than assumed: building the
same 10M-row frame both ways and diffing gives 0 differing rows, and the same
holds on the walkthrough model.

One test moved. examples/walkthrough.py printed `sol.primal('p').head(6)`
unsorted, so its golden file pinned the physical storage order of var_p —
which this change alters even though the labels do not. primal() is a label
join and a join has no inherent row order, so the example now sorts
explicitly. That makes the golden stable under any future storage change
(chunking, parallelism, a duckdb upgrade), which it was not before.

Left alone deliberately: `primal()` itself still returns rows in whatever
order the join produces. Adding an ORDER BY there would also hit
_solution_to_parquet, which streams, and a global sort is exactly what that
path exists to avoid.


Claude-Session: https://claude.ai/code/session_0145rKirxsCRGodhkKozjaYW

Co-authored-by: Claude <noreply@anthropic.com>

* perf: cols.vtype is an ENUM, not a VARCHAR per column (#174)

`vtype` is one of three literals written once per column, which as VARCHAR
made it the widest thing on the row — and the row count is the model's
column count. An ENUM stores the tag instead.

Nothing downstream moves: `vtype = '...'` comparisons in both sinks keep
working against an ENUM unchanged.

Measured on the branch this was split out of (#153): 0.87s -> 0.48s writing
`cols` at 10M columns.

The members come off `plan.VariableType` rather than being typed out, so the
Literal stays the single declaration. The test pins that: an ENUM rejects a
value it does not know, so a fourth variable type added to the plan and not
here would otherwise fail at insert with a duckdb cast error a long way from
its cause.

Closes #170.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(main): release 0.0.0-alpha.29 (#227)

Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com>

* perf: unmasked label assignment needs no sort (#152)

`_label_frame` numbered every coordinate with
`ROW_NUMBER() OVER (ORDER BY ord, ...)`, a full sort of every chunk. It was
71% of build at 10M variables (#146), and the sort is the whole cost: the
same statement without the ORDER BY runs at the join floor.

When nothing is masked out, that rank has a closed form -- it is the
mixed-radix value of the `ord` tuple -- so the label can be computed instead
of sorted for. Isolated at 10M rows: 4.37s -> 0.56s. End to end, interleaved
best-of-three, `transport` at 9.8M variables: build 10.19s -> 6.52s (-36%).

The labels are *the same integers*, not merely valid ones, which is what
makes this a pure optimisation rather than a decision. #151 weighed dropping
the ordering guarantee instead and had to leave three questions open --
`solver_direct`'s batching, reproducibility (#109), and the locality of `A`.
None of them arise here: nothing downstream can tell the two apart, and the
walkthrough golden that a renumbering would have forced to regenerate is
untouched.

Masked frames keep the window. Denseness there depends on which rows survive
the predicate, and no closed form knows that -- so `dispatch`, whose variable
is masked by `where: p_max > 0`, is unchanged by this commit.

Verified: HiGHS `addRows` is insensitive to column-index order within a row
(bit-identical objective and primal under reversed and shuffled indices), so
the `ORDER BY` was not load-bearing for the shipped path either.

332 passed; ruff and pyrefly clean.


Claude-Session: https://claude.ai/code/session_013DtGqM3z2DTtT556178iXi

Co-authored-by: Claude <noreply@anthropic.com>

* chore(main): release 0.0.0-alpha.30 (#228)

Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com>

* docs(bench): re-run the ladder on the merged tree

Full ladder after merging main: five cases, two sinks, both arms, best of
three, parity gate green on all five. 288 records, no failures.

**Every ratio is unchanged within noise, and that is the expected result.**
main's three duckdb perf PRs cannot move this comparison: neither arm here is
duckdb. The farkas arm is the polars engine and the linopy arm is
`farkas.linopy.build`, so the work in #152, #174 and #178 has nothing to act
on — their ideas already arrived on this branch as `_positional`, #211 and
#212, and it is those that the numbers already reflected.

`transport` reads 1.59x / 0.81x where it read 1.62x / 0.78x, `sector` 0.31x
where it read 0.32x, and so on. The tables and every figure quoted in the
prose are refreshed off the new file so the doc and its provenance agree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test: route the objective tests' pandas through the oracle guard

`test_objective.py` arrived from main with a bare `import pandas as pd`. It
merged without a conflict, so nothing flagged it — and on this branch pandas
is not a core dependency, so the bare-install CI job died collecting it.

Every other test module on this branch takes pandas from `tests.oracle`, which
is a `pytest.importorskip` behind the `[linopy]` extra. `test_duals.py` even
carries the comment this file needed: "through the guard: a bare import would
beat it".

Reproduced against an install with no pandas, linopy or xarray: 11 skipped
and a collection error before, 214 passed and 21 skipped after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine:polars Specific to the polars rewrite (#189) — invalidated if duckdb stays

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant