Skip to content

perf: collect relationship projection fields during query construction - #4

Open
jechol wants to merge 1 commit into
mainfrom
perf/collect-projection-fields
Open

jechol wants to merge 1 commit into
mainfrom
perf/collect-projection-fields

Conversation

@jechol

@jechol jechol commented Nov 10, 2026

Copy link
Copy Markdown
Owner

This replaces the implementation in ash-project/ash_sql#281, following the suggestion to collect required fields while building the query instead of scanning expressions at the end.

Each prunable from_many? join gets a token. Field references and correlation keys are recorded against that token during query construction. The final pass uses this metadata to narrow implicit selections, without traversing expression ASTs or resolving aliases.

When dependencies cannot be tracked reliably—including opaque SQL fragments, backend expressions, and modify_query callbacks—the original selection is retained. There is no AST-scanning fallback.

The execution integration is in ash-project/ash_postgres#919.

Validation:

  • 47 focused AshSql tests and 382 focused AshPostgres tests pass, including local companion tests for correlated expressions and modify_query.
  • Invoice, quotation, and contract list/detail reads were checked in a private application: 135 query preparations with matching results and bind parameters.
  • End-to-end data-loading timings were roughly unchanged; this is primarily a SQL readability improvement.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-10T10:11:53.281704Z c434539 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

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

Copy link
Copy Markdown

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: c434539e32

ℹ️ 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".

Comment thread lib/aggregate.ex
Comment on lines +42 to +44
%{relationship_path: [first | _]}, query ->
relationship = Ash.Resource.Info.relationship(resource, first)
AshSql.Query.Projection.track_source(query, binding, relationship)

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 Disable projection pruning for manual aggregate relationships

When an aggregate is attached to a from_many? binding and its first relationship is manual, recording only source_attribute does not capture the callback's dependencies. AshSql.Aggregate.Lateral.apply_relationship_subquery/8 invokes the manual relationship callback directly without marking its accumulator unknown, unlike the manual join and EXISTS paths. For example, a manual count relationship that correlates using another parent column can have that column removed from the source subquery, causing a missing-column SQL error. Mark dependencies unknown for manual aggregate relationships rather than treating their declared source attribute as sufficient.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant