Skip to content

[iris] Remove ORM query builder, replace with raw SQL#4181

Merged
rjpower merged 2 commits intomainfrom
rjpower/20260326-iris-opt
Mar 27, 2026
Merged

[iris] Remove ORM query builder, replace with raw SQL#4181
rjpower merged 2 commits intomainfrom
rjpower/20260326-iris-opt

Conversation

@rjpower
Copy link
Copy Markdown
Collaborator

@rjpower rjpower commented Mar 26, 2026

Delete Table, Column, Predicate, SelectExpr, Order, Join, JoinedQuery
and all 16 module-level table constants from db.py. Replace ~100
callsites across 8 production files and 12 test files with raw SQL +
decode_rows/decode_one helpers. Every query is now visible as plain SQL
at its callsite. Net -556 lines.

Follows #4179 which fixed the immediate scheduling perf issue.

@rjpower rjpower added the agent-generated Created by automation/agent label Mar 26, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 26, 2026

Claude finished @rjpower's task in 6m 31s —— View job


PR Review: [iris] Remove ORM query builder, replace with raw SQL

  • Gather context and read changed files
  • Review for correctness and behavioral regressions
  • Post final review

Clean mechanical removal of the ORM query builder (~550 lines net deletion) replacing ~100 callsites with raw SQL + decode_rows/decode_one helpers. The stated objective is met. Every query is now visible as plain SQL at its callsite.

Findings:

  • endpoint_query_sql self-join on endpoints table (db.py:522-523): JOIN endpoints et ON e.endpoint_id = et.endpoint_id is a self-join that can produce duplicate rows when an endpoint has multiple task rows. This is not a regression -- the old ENDPOINT_TASKS table object pointed at the same endpoints table with alias et and produced identical SQL. Worth a SELECT DISTINCT e.* or a comment noting the pre-existing behavior, but out of scope for this PR.

  • tasks_for_job_with_attempts ORDER BY changed (db.py:909): Old code: ORDER BY job_id, task_index. New code: ORDER BY task_id, task_index. Since the WHERE filters to a single job_id, the old job_id sort was a no-op and the effective sort was task_index ASC. The new sort uses task_id (string sort) as primary key, which could order task-10 before task-2. In practice task_index is still present as a tiebreaker, and task_id values for a single job likely sort the same as task_index for small task counts, but this is a subtle behavioral change. Consider ORDER BY task_index ASC, task_id ASC to preserve the original effective ordering.

  • Private helpers exported cross-module (autoscaler.py:50): _decode_json_list and _decode_timestamp_ms are _-prefixed private functions in db.py now imported by autoscaler.py. Not a bug, but consider dropping the underscore prefix since they're now part of the module's public API surface.

  • TransactionCursor.insert/update/delete removed: These convenience methods are gone and all callsites now use cur.execute(...) with hand-written SQL. This is consistent with the PR's goal. The TransactionCursor class is now just a thin wrapper around sqlite3.Cursor -- worth noting it could be replaced entirely with the raw cursor if desired in a follow-up.

No bugs, resource leaks, race conditions, or guideline violations found. The conversion is mechanically correct.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 59fa6cbabb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

tasks = decode_rows(
Task,
q.fetchall(
"SELECT * FROM tasks WHERE job_id = ? ORDER BY task_id, task_index",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Order tasks by numeric index for per-job reads

tasks_for_job_with_attempts now orders rows by task_id text, which changes the previous numeric task ordering to lexicographic ordering (e.g., .../10 comes before .../2) for jobs with 10+ replicas. This helper is used by job-status/listing paths, so task arrays can be returned in a misleading order and no longer align with task indices. Ordering should remain index-based (task_index) to preserve stable, numeric task sequence.

Useful? React with 👍 / 👎.

@rjpower rjpower requested a review from yonromai March 27, 2026 20:02
@rjpower
Copy link
Copy Markdown
Collaborator Author

rjpower commented Mar 27, 2026

@claude merge this branch with main/rebase & fix the conflict, address the concern from code review as well.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 27, 2026

Claude finished @rjpower's task in 5m 17s —— View job


Rebase & fix review concerns

  • Rebase branch on origin/main and resolve conflicts
  • Fix tasks_for_job_with_attempts ORDER BY (task_id, task_indextask_index, task_id)
  • Run pre-commit and tests
  • Push changes

Summary: Rebased on origin/main, resolved one conflict in db.py (removed stale ORM table constants that were re-added on main), and fixed the ORDER BY regression flagged by both Claude and Codex reviews — tasks now sort by task_index, task_id instead of task_id, task_index to preserve numeric ordering.

Testing: All 273 iris tests pass (212 from test_db/test_query/test_transitions/test_scheduler + 61 from test_heartbeat/test_direct_controller/test_dashboard/test_dry_run). Pre-commit passes clean.
·

rjpower and others added 2 commits March 27, 2026 20:06
Delete Table, Column, Predicate, SelectExpr, Order, Join, JoinedQuery
and all 16 module-level table constants from db.py. Replace ~100
callsites across 8 production files and 12 test files with raw SQL +
decode_rows/decode_one helpers. Every query is now visible as plain SQL
at its callsite. Net -556 lines.
Address code review: change ORDER BY from task_id,task_index to
task_index,task_id so tasks sort numerically rather than lexicographically.
Also resolve rebase conflict (removed stale ORM table constants from main).

Co-authored-by: Russell Power <rjpower@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude claude Bot force-pushed the rjpower/20260326-iris-opt branch from 59fa6cb to bd77a6e Compare March 27, 2026 20:09
@rjpower rjpower enabled auto-merge (squash) March 27, 2026 20:17
Copy link
Copy Markdown
Contributor

@yonromai yonromai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 I re-reviewed the current PR head after the task-ordering follow-up commit and re-ran the targeted controller/query suites. I did not find a merge-blocking behavior regression in the raw-SQL rewrite, so this is safe to approve.

Non-blocking follow-up: lib/iris/docs/sql-canonical.md still documents the removed query DSL (q.select(), q.one(), q.count(), tx.insert()/update()/delete()). That doc should be updated in a follow-up so it matches the new explicit-SQL API surface.

Generated with Codex

@rjpower rjpower merged commit bbaef73 into main Mar 27, 2026
42 checks passed
@rjpower rjpower deleted the rjpower/20260326-iris-opt branch March 27, 2026 20:47
Helw150 pushed a commit that referenced this pull request Apr 8, 2026
Delete Table, Column, Predicate, SelectExpr, Order, Join, JoinedQuery
and all 16 module-level table constants from db.py. Replace ~100
callsites across 8 production files and 12 test files with raw SQL +
decode_rows/decode_one helpers. Every query is now visible as plain SQL
at its callsite. Net -556 lines.

Follows #4179 which fixed the immediate scheduling perf issue.

---------

Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Russell Power <rjpower@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-generated Created by automation/agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants