Skip to content

Work on inlined primitive collections (VALUES) - #36159

Merged
roji merged 1 commit into
dotnet:mainfrom
roji:PruneValues
Jul 30, 2025
Merged

Work on inlined primitive collections (VALUES)#36159
roji merged 1 commit into
dotnet:mainfrom
roji:PruneValues

Conversation

@roji

@roji roji commented May 29, 2025

Copy link
Copy Markdown
Member

Work on inlined primitive collections (VALUES)

  • Refactor and generalize the pruning of the VALUES _ord column,
    moving it from RelationalTypeMappingPostprocessor to SqlPruner.
  • Ensure that inlined parameterized collections (EF.Constant)
    preserved ordering via _ord and specifies the type mapping.
  • Properly visit JsonScalarExpression in SqlNullabilityProcessor.
  • Add CAST() to VALUES generated in SqlNullabilityProcessor because
    of EF.Constant().

Fixes #36158
Fixes #36462
Fixes #36463

/cc @artl93

@roji
roji requested a review from cincuranet May 29, 2025 12:05
@roji
roji marked this pull request as ready for review May 30, 2025 15:51
@roji
roji requested a review from a team as a code owner May 30, 2025 15:51
@roji
roji enabled auto-merge (squash) May 31, 2025 07:23
cincuranet
cincuranet previously approved these changes Jul 9, 2025
@roji
roji requested a review from AndriySvyryd as a code owner July 9, 2025 10:25
@roji
roji dismissed cincuranet’s stale review July 29, 2025 17:57

Brought up to date and made various changes, re-review needed.

@roji roji left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@cincuranet I've brought this back from the dead - rebased on latest main and uncovered a couple of extra bugs which I fixed. Can you please give this another review?

Comment thread src/EFCore.Relational/Query/SqlNullabilityProcessor.cs
var intTypeMapping = (IntTypeMapping?)Dependencies.TypeMappingSource.FindMapping(typeof(int));
Check.DebugAssert(intTypeMapping is not null);
var valuesOrderingCounter = 1;
var valuesOrderingCounter = 0;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@cincuranet I did this tiny change to align our ordering counter to start at zero only because that's what we do in translation for regular ValuesExpression (where there's no EF.Constant() etc.). Of course the actual values here don't matter - we only order by them (we can also go the other way and start with 1 ;)).

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.

I started with 1 because in SQL stuff is usually indexed from 1. But I don't feel strongly about it.

Comment thread src/EFCore.Relational/Query/SqlNullabilityProcessor.cs
* Refactor and generalize the pruning of the VALUES _ord column,
  moving it from RelationalTypeMappingPostprocessor to SqlPruner.
* Ensure that inlined parameterized collections (EF.Constant)
  preserved ordering via _ord and specifies the type mapping.
* Properly visit JsonScalarExpression in SqlNullabilityProcessor.
* Add CAST() to VALUES generated in SqlNullabilityProcessor because
  of EF.Constant().

Fixes dotnet#36158
Fixes dotnet#36462
Fixes dotnet#36463
var intTypeMapping = (IntTypeMapping?)Dependencies.TypeMappingSource.FindMapping(typeof(int));
Check.DebugAssert(intTypeMapping is not null);
var valuesOrderingCounter = 1;
var valuesOrderingCounter = 0;

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.

I started with 1 because in SQL stuff is usually indexed from 1. But I don't feel strongly about it.

@roji
roji merged commit 0b46928 into dotnet:main Jul 30, 2025
7 checks passed
@roji
roji deleted the PruneValues branch July 30, 2025 08:41
@roji

roji commented Jul 30, 2025

Copy link
Copy Markdown
Member Author

I started with 1 because in SQL stuff is usually indexed from 1. But I don't feel strongly about it.

Me neither - perfectly fine to align to 1 in both cases, just want them to be consistent is all.

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

Labels

None yet

Projects

None yet

2 participants