Skip to content

preserve visitor rewrites in window function SQL expressions#79

Open
koenbeuk wants to merge 1 commit into
mainfrom
fix/window-sqlexpression-visitors
Open

preserve visitor rewrites in window function SQL expressions#79
koenbeuk wants to merge 1 commit into
mainfrom
fix/window-sqlexpression-visitors

Conversation

@koenbeuk

Copy link
Copy Markdown
Collaborator

WindowFunctionSqlExpression.VisitChildren served two masters: rendering SQL for
QuerySqlGenerator and answering EF's rewrite passes — and it discarded the
latter's results, so alias remapping and pushdown silently lost their rewrites.
Now self-renders only for QuerySqlGenerator and rebuilds children for every
other visitor. Also chains SqlExpression's Type/TypeMapping into equality on
all three window expression types, and makes the nullability-wrapper unwrap
idempotent (duplicated placeholders no longer leak literal __wf_placeholder_N
into the SQL).

Copilot AI review requested due to automatic review settings July 26, 2026 01:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a correctness gap in the EF Core relational window-function SQL expression types: WindowFunctionSqlExpression.VisitChildren previously self-rendered in a way that prevented non-SQL-generation visitors (e.g., alias remapping and projection pushdown) from preserving their rewrites. The update keeps self-rendering only for QuerySqlGenerator while ensuring all other visitors rebuild children normally, and it tightens equality/hash semantics to include SqlExpression base state (CLR Type and TypeMapping). It also makes the placeholder unwrap logic idempotent so duplicated placeholders no longer leak into final SQL.

Changes:

  • Rework WindowFunctionSqlExpression.VisitChildren to rebuild rewritten children for non-QuerySqlGenerator visitors while retaining self-rendering for SQL generation.
  • Include SqlExpression base equality/hash (Type/TypeMapping) in window expression equality across the three window expression types.
  • Make window-function placeholder unwrapping idempotent and add targeted tests covering visitor rewrites, equality, and duplicated placeholder restoration.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/ExpressiveSharp.Tests/RelationalExtensions/WindowFunctionSqlExpressionTests.cs Adds unit tests ensuring visitor rewrites are preserved, equality respects CLR type, and duplicated placeholders unwrap correctly.
src/ExpressiveSharp.EntityFrameworkCore.RelationalExtensions/Infrastructure/Internal/WindowSpecSqlExpression.cs Extends equality/hash to include SqlExpression base state (Type/TypeMapping).
src/ExpressiveSharp.EntityFrameworkCore.RelationalExtensions/Infrastructure/Internal/WindowFunctionSqlExpressionWrapper.cs Makes placeholder unwrapping non-destructive so the same placeholder can be restored multiple times.
src/ExpressiveSharp.EntityFrameworkCore.RelationalExtensions/Infrastructure/Internal/WindowFunctionSqlExpression.cs Updates VisitChildren to preserve rewrite results for non-SQL visitors; extends equality/hash to include base state.
src/ExpressiveSharp.EntityFrameworkCore.RelationalExtensions/Infrastructure/Internal/WindowFrameBoundSqlExpression.cs Extends equality/hash to include SqlExpression base state (Type/TypeMapping).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...tructure/Internal/WindowFrameBoundSqlExpression.cs 50.00% 0 Missing and 1 partial ⚠️
...astructure/Internal/WindowFunctionSqlExpression.cs 96.00% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

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.

2 participants