Make the #30915 nullability marker survive a subsequent join#38499
Open
ajcvickers wants to merge 1 commit into
Open
Make the #30915 nullability marker survive a subsequent join#38499ajcvickers wants to merge 1 commit into
ajcvickers wants to merge 1 commit into
Conversation
Fixes dotnet#26873 Builds on dotnet#30915 (a synthesized marker column that materializes a left-joined non-entity projection as `null` on a no-match). dotnet#30915 covered a single left join; this makes the marker survive a *subsequent* join. - **Root cause:** a second `AddJoin` remaps the outer shaper and rebuilds the `New`/`MemberInit` node a prior join recorded as a marker key, orphaning the reference-keyed marker — so the gate never fires and the no-match row throws *"Nullable object must have a value"*. - **Fix:** capture the nodes rebuilt by the outer-shaper remap (`RebuiltNodes`) and re-key each affected marker onto its new node with a rebound binding. Localized to `SelectExpression` / `ProjectionMemberRemappingExpressionVisitor`. - **Fail-safe, no regressions:** gated on `_nonEntityNullabilityMarkers is not null`, a pure no-op for queries without a dotnet#30915 marker. If a marker's projection member was pruned, the re-key is skipped and behavior degrades to the prior throw — never an incorrect result or a new crash. - **Generalizes:** the re-key composes across multiple sequential joins and through nested anonymous wrappers. - **Tests:** flipped `Second_join_after_then_whole_object` and `Two_left_joined_nonentity_objects_second_marker_orphaned` to assert correct results; added three-sequential-joins (re-key composition) and nested-wrapper characterization tests. SQLite + SQL Server, zero baseline drift. Sub-case of the umbrella dotnet#22517. Remaining follow-ups — GroupBy-after (dotnet#28119), plain-inner/no-pushdown, value-type/`Nullable<T>`, set-operations, server-side null checks, RightJoin/outer-nullable, InMemory parity — stay deferred and remain pinned by characterization tests.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a relational query-shaping bug where the #30915 synthesized “nullability marker” (used to materialize left-joined non-entity projections as null on no-match rows) could be orphaned by a subsequent join, causing incorrect materialization behavior (or the prior exception behavior).
Changes:
- Track
New/MemberInitnodes rebuilt during outer-shaper projection-member remapping and use that information to keep previously-recorded non-entity nullability markers correctly keyed after subsequent joins. - Re-key affected
_nonEntityNullabilityMarkersentries duringSelectExpression.AddJoinouter-shaper remaps, rebinding the marker through the same remap when possible. - Update/add relational/spec tests and provider SQL assertions (SqlServer/Sqlite) to validate correct null materialization across multiple joins and nested wrapper shapes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs | Adds SQL assertions for new/updated regression tests covering marker survival across subsequent joins. |
| test/EFCore.Sqlite.FunctionalTests/Query/AdHocMiscellaneousQuerySqliteTest.cs | Adds SQL assertions for the same regression scenarios on SQLite. |
| test/EFCore.Relational.Specification.Tests/Query/AdHocMiscellaneousQueryRelationalTestBase.cs | Converts prior “throws” characterization to result assertions; adds additional join-remap survival scenarios. |
| src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs | Extends the projection-member remapping visitor to record rebuilt New/MemberInit nodes for marker re-keying. |
| src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs | Uses rebuilt-node tracking to re-key non-entity nullability markers after outer-shaper remaps in AddJoin. |
Comment on lines
+57
to
+61
| // #30915: tracks New/MemberInit nodes rebuilt by this visitor (old instance → new instance). Used by the caller | ||
| // to re-key any _nonEntityNullabilityMarkers entries whose key was one of the rebuilt nodes, so that a previously- | ||
| // recorded marker key does not go stale when an outer-shaper remap creates a fresh node instance. | ||
| public Dictionary<Expression, Expression> RebuiltNodes { get; } | ||
| = new Dictionary<Expression, Expression>(ReferenceEqualityComparer.Instance); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #26873
Builds on #30915 (a synthesized marker column that materializes a left-joined non-entity projection as
nullon a no-match). #30915 covered a single left join; this makes the marker survive a subsequent join.AddJoinremaps the outer shaper and rebuilds theNew/MemberInitnode a prior join recorded as a marker key, orphaning the reference-keyed marker.RebuiltNodes) and re-key each affected marker onto its new node with a rebound binding._nonEntityNullabilityMarkers is not null, a pure no-op for queries without a Nullable object must have a value thrown for a query with DefaultIfEmpty() #30915 marker.Sub-case of the umbrella #22517. Remaining follow-ups — GroupBy-after (#28119), plain-inner/no-pushdown, value-type/
Nullable<T>, set-operations, server-side null checks, RightJoin/outer-nullable, InMemory parity — stay deferred and remain pinned by characterization tests.