refactor: three modules in the engine, one per box in the diagram - #107
Conversation
|
Warning Review limit reached
Next review available in: 33 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 (10)
📝 WalkthroughWalkthroughThe relational lane is split into a pure SQL compiler, a DuckDB executor responsible for table assembly, and sink functions responsible for LP output and direct solving. Compiler-focused tests and architecture documentation are updated accordingly. ChangesRelational lane separation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Program
participant SqlCompiler
participant DuckdbExecutor
participant Sinks
participant DuckDB
participant HiGHS
Program->>DuckdbExecutor: build relational tables
DuckdbExecutor->>SqlCompiler: compile plan expressions and predicates
SqlCompiler-->>DuckdbExecutor: SQL fragments
DuckdbExecutor->>Sinks: provide ModelTables
Sinks->>DuckDB: read assembled model tables
Sinks->>HiGHS: stream variables and constraint batches
HiGHS-->>DuckDB: write solver solution table
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@linopy_yaml/relational/compiler.py`:
- Around line 342-350: Update _comparison_sql to escape single quotes in string
literals before wrapping them in SQL quotes, so values such as O'Brien produce
valid SQL. Preserve numeric literal handling and the existing operator
translation unchanged.
- Around line 250-258: Update _sum_fragment to validate every dimension in over
before indexing self.dimension_cardinality, and raise LanguageError for any
undeclared dimension with a clear load-time diagnostic. Preserve the existing
missing-dimension mask validation and cardinality scaling for declared
dimensions.
In `@linopy_yaml/relational/sinks.py`:
- Around line 214-229: Guard the solution-table construction after computing
status and objective in the solve result path: only build and register the `sol`
DataFrame when `h.getSolution().col_value` contains a complete primal vector
matching `model.column_count`. For infeasible, unbounded, or otherwise
solution-less results, skip the DataFrame and table creation while still
returning the computed status and objective.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2830bb54-db70-45a8-a0ed-f3ce039504b0
📒 Files selected for processing (6)
ARCHITECTURE.mdlinopy_yaml/relational/compiler.pylinopy_yaml/relational/executor.pylinopy_yaml/relational/sinks.pytests/test_architecture.pytests/test_compiler.py
| def _sum_fragment(self, p: TermFragment, over: tuple[str, ...], context: str) -> TermFragment: | ||
| missing = [d for d in over if d not in p.dims] | ||
| if missing and not p.is_term: | ||
| raise LanguageError( | ||
| f'in {context}: Sum over {list(over)} of a constant part lacking dims ' | ||
| f'{missing} is ambiguous under masks — multiply explicitly instead' | ||
| ) | ||
| keep = tuple(d for d in p.dims if d not in over) | ||
| scale = math.prod(self.dimension_cardinality[d] for d in missing) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unknown dim in Sum(over=…) raises KeyError, not a LanguageError.
self.dimension_cardinality[d] is indexed for every missing dim. If over names a dimension that no declaration carries, the cardinality map has no entry and the user sees a bare KeyError instead of the load-time diagnostic this module produces everywhere else.
🛠️ Proposed guard
keep = tuple(d for d in p.dims if d not in over)
+ unknown = [d for d in missing if d not in self.dimension_cardinality]
+ if unknown:
+ raise LanguageError(f'in {context}: Sum over unknown dimension(s) {unknown}')
scale = math.prod(self.dimension_cardinality[d] for d in missing)As per coding guidelines, "Perform all validation at load time and produce clear, actionable error messages."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _sum_fragment(self, p: TermFragment, over: tuple[str, ...], context: str) -> TermFragment: | |
| missing = [d for d in over if d not in p.dims] | |
| if missing and not p.is_term: | |
| raise LanguageError( | |
| f'in {context}: Sum over {list(over)} of a constant part lacking dims ' | |
| f'{missing} is ambiguous under masks — multiply explicitly instead' | |
| ) | |
| keep = tuple(d for d in p.dims if d not in over) | |
| scale = math.prod(self.dimension_cardinality[d] for d in missing) | |
| def _sum_fragment(self, p: TermFragment, over: tuple[str, ...], context: str) -> TermFragment: | |
| missing = [d for d in over if d not in p.dims] | |
| if missing and not p.is_term: | |
| raise LanguageError( | |
| f'in {context}: Sum over {list(over)} of a constant part lacking dims ' | |
| f'{missing} is ambiguous under masks — multiply explicitly instead' | |
| ) | |
| keep = tuple(d for d in p.dims if d not in over) | |
| unknown = [d for d in missing if d not in self.dimension_cardinality] | |
| if unknown: | |
| raise LanguageError(f'in {context}: Sum over unknown dimension(s) {unknown}') | |
| scale = math.prod(self.dimension_cardinality[d] for d in missing) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@linopy_yaml/relational/compiler.py` around lines 250 - 258, Update
_sum_fragment to validate every dimension in over before indexing
self.dimension_cardinality, and raise LanguageError for any undeclared dimension
with a clear load-time diagnostic. Preserve the existing missing-dimension mask
validation and cardinality scaling for declared dimensions.
Source: Coding guidelines
| def _comparison_sql(column: str, op: plan.ComparisonOperator, value: float | str) -> str: | ||
| """One where-comparison: ``(<column> <op> <literal>)``. | ||
|
|
||
| The language's ``==`` is SQL's ``=``, and a string literal needs quoting — | ||
| stated once, since the parameter and dimension cases differ only in which | ||
| column they test. | ||
| """ | ||
| literal = f"'{value}'" if isinstance(value, str) else repr(value) | ||
| return f'({column} {"=" if op == "==" else op} {literal})' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Single quotes in string values are not escaped.
A where-comparison against a label containing ' (e.g. where: name == "O'Brien") emits malformed SQL and surfaces as a duckdb parse error rather than a model-level message.
🛠️ Proposed fix
- literal = f"'{value}'" if isinstance(value, str) else repr(value)
+ literal = "'" + value.replace("'", "''") + "'" if isinstance(value, str) else repr(value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _comparison_sql(column: str, op: plan.ComparisonOperator, value: float | str) -> str: | |
| """One where-comparison: ``(<column> <op> <literal>)``. | |
| The language's ``==`` is SQL's ``=``, and a string literal needs quoting — | |
| stated once, since the parameter and dimension cases differ only in which | |
| column they test. | |
| """ | |
| literal = f"'{value}'" if isinstance(value, str) else repr(value) | |
| return f'({column} {"=" if op == "==" else op} {literal})' | |
| def _comparison_sql(column: str, op: plan.ComparisonOperator, value: float | str) -> str: | |
| """One where-comparison: ``(<column> <op> <literal>)``. | |
| The language's ``==`` is SQL's ``=``, and a string literal needs quoting — | |
| stated once, since the parameter and dimension cases differ only in which | |
| column they test. | |
| """ | |
| literal = "'" + value.replace("'", "''") + "'" if isinstance(value, str) else repr(value) | |
| return f'({column} {"=" if op == "==" else op} {literal})' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@linopy_yaml/relational/compiler.py` around lines 342 - 350, Update
_comparison_sql to escape single quotes in string literals before wrapping them
in SQL quotes, so values such as O'Brien produce valid SQL. Preserve numeric
literal handling and the existing operator translation unchanged.
| status = str(h.getModelStatus()).rsplit('.', 1)[-1].removeprefix('k') | ||
| objective = h.getInfo().objective_function_value + model.objective_constant | ||
|
|
||
| import pandas as pd | ||
|
|
||
| primal = pd.DataFrame( | ||
| { | ||
| 'col': np.arange(model.column_count, dtype=np.int64), | ||
| 'value': np.asarray(h.getSolution().col_value, dtype=np.float64), | ||
| } | ||
| ) | ||
| con.execute('DROP TABLE IF EXISTS sol') | ||
| con.register('sol_src', primal) | ||
| con.execute('CREATE TABLE sol AS SELECT * FROM sol_src') | ||
| con.unregister('sol_src') | ||
| return status, objective |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Infeasible/unbounded solves raise instead of returning their status.
status and objective are computed first, but the sol DataFrame is then built from h.getSolution().col_value zipped against np.arange(column_count). When HiGHS has no primal solution (infeasible, unbounded, iteration/time limit), col_value is empty and pd.DataFrame fails with a length-mismatch ValueError — so the caller never sees the Infeasible status this function otherwise computes correctly.
🛠️ Proposed guard
- primal = pd.DataFrame(
- {
- 'col': np.arange(model.column_count, dtype=np.int64),
- 'value': np.asarray(h.getSolution().col_value, dtype=np.float64),
- }
- )
- con.execute('DROP TABLE IF EXISTS sol')
- con.register('sol_src', primal)
- con.execute('CREATE TABLE sol AS SELECT * FROM sol_src')
- con.unregister('sol_src')
+ values = np.asarray(h.getSolution().col_value, dtype=np.float64)
+ con.execute('DROP TABLE IF EXISTS sol')
+ if len(values) == model.column_count:
+ primal = pd.DataFrame({'col': np.arange(model.column_count, dtype=np.int64), 'value': values})
+ con.register('sol_src', primal)
+ con.execute('CREATE TABLE sol AS SELECT * FROM sol_src')
+ con.unregister('sol_src')
return status, objectiveWorth a pytest case that solves a trivially infeasible model and asserts the returned status.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| status = str(h.getModelStatus()).rsplit('.', 1)[-1].removeprefix('k') | |
| objective = h.getInfo().objective_function_value + model.objective_constant | |
| import pandas as pd | |
| primal = pd.DataFrame( | |
| { | |
| 'col': np.arange(model.column_count, dtype=np.int64), | |
| 'value': np.asarray(h.getSolution().col_value, dtype=np.float64), | |
| } | |
| ) | |
| con.execute('DROP TABLE IF EXISTS sol') | |
| con.register('sol_src', primal) | |
| con.execute('CREATE TABLE sol AS SELECT * FROM sol_src') | |
| con.unregister('sol_src') | |
| return status, objective | |
| status = str(h.getModelStatus()).rsplit('.', 1)[-1].removeprefix('k') | |
| objective = h.getInfo().objective_function_value + model.objective_constant | |
| import pandas as pd | |
| values = np.asarray(h.getSolution().col_value, dtype=np.float64) | |
| con.execute('DROP TABLE IF EXISTS sol') | |
| if len(values) == model.column_count: | |
| primal = pd.DataFrame( | |
| { | |
| 'col': np.arange(model.column_count, dtype=np.int64), | |
| 'value': values, | |
| } | |
| ) | |
| con.register('sol_src', primal) | |
| con.execute('CREATE TABLE sol AS SELECT * FROM sol_src') | |
| con.unregister('sol_src') | |
| return status, objective |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@linopy_yaml/relational/sinks.py` around lines 214 - 229, Guard the
solution-table construction after computing status and objective in the solve
result path: only build and register the `sol` DataFrame when
`h.getSolution().col_value` contains a complete primal vector matching
`model.column_count`. For infeasible, unbounded, or otherwise solution-less
results, skip the DataFrame and table creation while still returning the
computed status and objective.
ARCHITECTURE.md's pipeline has always drawn the relational lane as a plan, an
executor and two sinks. The code had one 1,019-line file and a
`DuckdbExecutor` with 32 methods that owned duckdb, HiGHS and LP
serialisation between them. This makes the picture true.
relational/compiler.py plan -> SQL text. Pure: no connection.
relational/executor.py owns the database; fills the tables.
relational/sinks/ drains them: lp_file, solver_direct.
The seam was already there to be found. Every method that built SQL —
`_compile`, `_pred_sql`, `_bound_sql`, `_frame_sql`, the three fragment
rewriters — read exactly three things: the program, each dimension's
cardinality, and which parameters are boolean. None of them touched the
connection. `SqlCompiler` is those three fields and those methods, so
compiling a plan node now needs no engine at all.
That is worth more than tidiness. ARCHITECTURE.md asks you to judge a new
operator by reading the SQL it emits — pointwise, bounded-halo or global.
Until now that meant building a model and solving it. `tests/test_compiler.py`
does it in 19 assertions with nothing installed: hand it `Translate` and check
the dim table is joined twice and no window appears; hand it `GroupSum` and
check the dim tuple changed and no `GROUP BY` did.
`test_every_ir_expr_node_is_handled_by_the_executor` becomes
`test_every_plan_node_is_handled_by_the_compiler` and greps the module that
actually consumes the nodes — the executor no longer mentions most of them,
and a node the compiler ignores has no relational meaning however much the
executor moves around it.
Sinks are a package, one module per sink, because that is where the fences
are: `highspy` is an optional dependency of `solver_direct` alone, and a
caller who only writes LP files should not import it (verified — importing
`sinks.lp_file` leaves `highspy` out of `sys.modules`). Splitting by kind
instead — writers together, solvers together — would put two optional imports
in one module and a function that branches on which solver you meant.
`sinks/README.md` carries the contract, the checklist for adding one, and why
`mps` is deliberately not pre-grouped with `lp_file` yet.
`test_architecture_doc_mentions_every_module` becomes
`test_every_module_is_documented_somewhere`: a README beside the code counts.
Otherwise a subpackage with one member per variant pushes its whole membership
list into ARCHITECTURE.md's map, which is the thing that map exists not to be.
Pure move: no SQL text, no numerics and no error message changed. LP output is
identical block-for-block (it is not byte-stable run to run, on main either —
#109).
executor.py 1019 -> 595 lines. 308 tests pass, 19 of them new.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f0105b1 to
432b534
Compare
1 participant
ARCHITECTURE.md's pipeline has always drawn the relational lane as a plan, an executor and two sinks. The code had one 1,019-line file and aDuckdbExecutorwith 32 methods that owned duckdb, HiGHS and LP serialisation between them. This makes the picture true.relational/compiler.pyrelational/executor.pyrelational/sinks/lp_file,solver_direct— one module eachThe seam was already there
Every method that built SQL —
_compile,_pred_sql,_bound_sql,_frame_sql, the three fragment rewriters — read exactly three things: the program, each dimension's cardinality, and which parameters are boolean. None of them touchedself._con. SoSqlCompileris those three fields plus those methods, and compiling a plan node now needs no engine at all:The two facts that are data-dependent are why this is a dataclass rather than free functions:
sumover a dim the operand lacks scales by that dim's cardinality, anddefinedon a boolean parameter tests the value rather than its finiteness. The compiler is therefore constructed inbuild(), after the sources are loaded.Why it's worth more than tidiness
ARCHITECTURE.mdasks you to judge a candidate operator by reading the SQL it emits — pointwise, bounded-halo, or global. Until now that meant building a model and solving it.tests/test_compiler.pydoes it in 19 assertions with nothing installed:19 tests, 0.11s, no duckdb — so they run on the bare-install job too.
test_every_ir_expr_node_is_handled_by_the_executorbecomestest_every_plan_node_is_handled_by_the_compilerand greps the module that actually consumes the nodes. The executor no longer mentions most of them, and a node the compiler ignores has no relational meaning however much the executor moves around it.Sinks are a package, one module per sink
Split by fence, not by kind:
highspyis an optional dependency ofsolver_directalone, and a caller who only writes LP files should not import it (verified — importingsinks.lp_fileleaveshighspyout ofsys.modules). Grouping all solvers into one file instead would put two optional imports in one module and a function that branches on which solver you meant, which is the shapehelpers.pyrefuses for the language.Deliberately not pre-grouping for
mps. It will probably belong besidelp_file— both are chunkedCOPYofprintf'd rows into part files, concatenated bytewise — but atext.pyholding one function today is a guess about a sink that does not exist. The README records that, plus the Track 4 caveat: when sinks gain capability tables, keep the set closed likehelpers.BUILTINS, because an installed plugin that can change the answer toly.check(model, sink=...)is hard rule 5's failure mode one level down.test_architecture_doc_mentions_every_modulebecomestest_every_module_is_documented_somewhere— a README beside the code counts. Otherwise a subpackage with one member per variant pushes its whole membership list into ARCHITECTURE.md's module map, which is the thing that map exists not to be.write_lp_fileandsolve_directtake aModelTables— a connection holdingcols/obj/rows/A, plus the counts a writer chunks by and the objective sense and constant (which live outside the tables because a constant has no column to attach to). Nothing else. The plannedmpssink is now a function in that file rather than a third method on the executor, andwrite_lp/solveon the executor are three lines each.This is a pure move
No SQL text, no numerics and no error message changed. 308 tests pass, including the full differential suite against linopy and
test_walkthrough.py's line-for-line assertion of committed output.I also checked LP output directly, and it is identical block-for-block rather than byte-for-byte — because the LP writer is not byte-stable run to run on main either.
preserve_insertion_order=falseleaves constraint block order free, and theCOPY ... GROUP BY r.rowhas noORDER BY, so two consecutive runs of unmodifiedmainproduce different file hashes with identical content. Solvers do not care, but it does meanwrite_lpcannot be used for regression diffing. Filed as #109; deliberately not fixed here, since a move-only refactor should not change output.Verification
uv run pytest— 308 passed, 1 xfailed ·ruff checkandruff format --checkclean ·pyrefly check0 errors.🤖 Generated with Claude Code
Summary by CodeRabbit