Fix ValuesExpression pruning dropping outer columns referenced from VALUES cells - #38779
Fix ValuesExpression pruning dropping outer columns referenced from VALUES cells#38779yvesleguennec wants to merge 1 commit into
Conversation
…ALUES cells - Visit nested RowValues/ValuesParameter in PruneValues so embedded ColumnExpressions are registered before outer join projections are pruned - Fix column-name backfill to use ColumnNames[j] instead of repeating the first unreferenced column's name Fixes dotnet#38700
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Fixes VALUES pruning so nested outer-column references are retained.
Changes:
- Visits VALUES cells during pruning.
- Corrects pruned column-name backfilling.
- Adds relational regression coverage and a SQL Server baseline.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/EFCore.Relational/Query/SqlTreePruner.cs |
Corrects VALUES traversal and column pruning. |
test/EFCore.Relational.Specification.Tests/Query/AdHocQueryFiltersQueryRelationalTestBase.cs |
Adds the regression scenario. |
test/EFCore.SqlServer.FunctionalTests/Query/AdHocQueryFiltersQuerySqlServerTest.cs |
Verifies the generated SQL projection. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| referencedColumns[j] = true; | ||
| newColumnNames.Add(columnName); | ||
| newColumnNames.Add(values.ColumnNames[j]); |
| public class SqlTreePrunerTest | ||
| { | ||
| [Fact] | ||
| public void PruneValues_preserves_leading_column_names_when_pruning_a_later_column() |
There was a problem hiding this comment.
Unit tests for query code aren't very useful. Try to find a real query that hits this case
There was a problem hiding this comment.
Thanks — agreed that a unit test isn’t the right way to cover query behavior here.
To clarify scope on the secondary change and that test:
This PR’s fix for #38700 is visiting nested RowValues / ValuesParameter in PruneValues, so ColumnExpressions embedded in inline VALUES cells get registered before outer join projections are pruned. That path is covered by the relational AdHoc query-filter regression (the original repro).
Separately, in the same method, the loop that backfills leading column names when a later column is pruned was using the pruned column’s name instead of each surviving leading name (ColumnNames[j]). That’s a real correctness bug in the pruned ColumnNames list (misaligned names vs kept values). The same backfill pattern is already correct for OpenJSON in SqlServerSqlTreePruner. I noticed it while reading PruneValues for that bug; I never hit it via a customer query, but I’m keeping the one-liner because it’s still a bug.
Copilot asked for coverage of that backfill path. I added a synthetic unit test for it; given your feedback, I’ll drop that test rather than inventing an artificial LINQ scenario for a case that doesn’t show up in normal _ord-first VALUES pruning.
968c7c8 to
60b1d5b
Compare
Fixes #38700
SqlTreePruner.PruneValues(from #36159) copied VALUES row cells withoutVisit(), soColumnExpressions embedded in inline collections were neverregistered. Outer join projections were then over-pruned (e.g. only
Idprojected while
EXISTSstill referencedServiceId).Fix
Visit()nestedRowValues/ValuesParameterinPruneValues(same pattern as OpenJSON pruning)
ColumnNames[j]instead of repeating thefirst unreferenced column's name
Tests
ServiceIdremains in the join projectionNote: approach described on #38700; no maintainer reply after ~1.5 weeks, opening PR for review.