-
Notifications
You must be signed in to change notification settings - Fork 533
Query: Fixes LINQ.Contains to handle Memory Span implicit conversion introduced in C#13/DotNet9 #5477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Query: Fixes LINQ.Contains to handle Memory Span implicit conversion introduced in C#13/DotNet9 #5477
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,7 +49,9 @@ private static bool CanBeEvaluated(Expression expression) | |
| if (methodCallExpression != null) | ||
| { | ||
| Type type = methodCallExpression.Method.DeclaringType; | ||
| if (type == typeof(Enumerable) || type == typeof(Queryable) || type == typeof(CosmosLinq)) | ||
|
|
||
| // Aside from the known types, we also need to avoid partial eval for op_implicit methods, which are the implicit conversions of enum to memoryextension span types (introduced in c#13) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| if (type == typeof(Enumerable) || type == typeof(Queryable) || type == typeof(CosmosLinq) || methodCallExpression.Method.Name == "op_Implicit") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make the check tighter so that we only let through those on_implicit calls that we support.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For a check that's broader than what we support, please add negative coverage for those cases.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please also guard consts (for downstream success). |
||
| { | ||
| return false; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,22 @@ public static ParameterExpression NewParameter(string prefix, Type type, HashSet | |
| suffix++; | ||
| } | ||
| } | ||
|
|
||
| public static bool TryUnwrapSpanImplicitCast(Expression expression, out Expression result) | ||
| { | ||
| if (expression is MethodCallExpression methodCallExpression | ||
| && methodCallExpression.Method.Name == "op_Implicit" | ||
| && methodCallExpression.Method.DeclaringType is { IsGenericType: true } implicitCastDeclaringType | ||
| && implicitCastDeclaringType.GetGenericTypeDefinition() is var genericTypeDefinition | ||
| && (genericTypeDefinition == typeof(Span<>) || genericTypeDefinition == typeof(ReadOnlySpan<>))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debug Assert inside if assuming this check is done at top level. |
||
| { | ||
| result = methodCallExpression.Arguments[0]; | ||
| return true; | ||
| } | ||
|
|
||
| result = null; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| internal abstract class ExpressionSimplifier | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please rename the project, class namespaces to something suitable that indicate these are .NET C#14 specific tests.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Microsoft.Azure.Cosmos.Tests.C14 |
||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <IsTestProject>true</IsTestProject> | ||
| <TargetFramework>net9.0</TargetFramework> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> | ||
| <LangVersion>preview</LangVersion> | ||
| <Platform>AnyCPU</Platform> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\src\Microsoft.Azure.Cosmos.csproj" /> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.2.0" /> | ||
| <PackageReference Include="MSTest.TestAdapter" Version="1.3.2" /> | ||
| <PackageReference Include="MSTest.TestFramework" Version="1.3.2" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition=" '$(ProjectRef)' != 'True' "> | ||
| <PackageReference Include="Microsoft.Azure.Cosmos.Direct" Version="[$(DirectVersion)]" PrivateAssets="All" /> | ||
| <PackageReference Include="Microsoft.HybridRow" Version="[$(HybridRowVersion)]" PrivateAssets="All" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| | ||
| Microsoft Visual Studio Solution File, Format Version 12.00 | ||
| # Visual Studio Version 17 | ||
| VisualStudioVersion = 17.0.31903.59 | ||
| MinimumVisualStudioVersion = 10.0.40219.1 | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.Azure.Cosmos", "..\..\src\Microsoft.Azure.Cosmos.csproj", "{1E7BA935-5548-458C-84E7-2F4CEFC620EC}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "CosmosDbDemo", "CosmosDbDemo.csproj", "{93FEBE02-3010-4367-8163-BC2FBE31E48C}" | ||
| EndProject | ||
| Global | ||
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | ||
| Debug|Any CPU = Debug|Any CPU | ||
| Debug|x64 = Debug|x64 | ||
| Debug|x86 = Debug|x86 | ||
| Release|Any CPU = Release|Any CPU | ||
| Release|x64 = Release|x64 | ||
| Release|x86 = Release|x86 | ||
| EndGlobalSection | ||
| GlobalSection(ProjectConfigurationPlatforms) = postSolution | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Debug|x64.ActiveCfg = Debug|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Debug|x64.Build.0 = Debug|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Debug|x86.ActiveCfg = Debug|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Debug|x86.Build.0 = Debug|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Release|x64.ActiveCfg = Release|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Release|x64.Build.0 = Release|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Release|x86.ActiveCfg = Release|Any CPU | ||
| {1E7BA935-5548-458C-84E7-2F4CEFC620EC}.Release|x86.Build.0 = Release|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Debug|x64.ActiveCfg = Debug|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Debug|x64.Build.0 = Debug|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Debug|x86.ActiveCfg = Debug|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Debug|x86.Build.0 = Debug|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Release|x64.ActiveCfg = Release|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Release|x64.Build.0 = Release|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Release|x86.ActiveCfg = Release|Any CPU | ||
| {93FEBE02-3010-4367-8163-BC2FBE31E48C}.Release|x86.Build.0 = Release|Any CPU | ||
| EndGlobalSection | ||
| GlobalSection(SolutionProperties) = preSolution | ||
| HideSolutionNode = FALSE | ||
| EndGlobalSection | ||
| EndGlobal |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| using System.Text.Json; | ||
| using System.Text.Json.Serialization; | ||
| using Microsoft.Azure.Cosmos; | ||
| using Microsoft.Azure.Cosmos.Linq; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
|
||
| [TestClass] | ||
| public class Program | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please rename, turn this into a test project (if not already). |
||
| { | ||
| // Cosmos DB Emulator values | ||
| private static readonly string EndpointUrl = "https://localhost:8081"; | ||
| private static readonly string PrimaryKey = "C2y6yDjf5/R+ob0N8A7Cgv30VRDJIWEHLM+4QDU5DE2nQ9nDuVTqobD4b8mGGyPMbIZnqyMsEcaGQy67XIw/Jw=="; | ||
| private static readonly string DatabaseId = "ToDoList"; | ||
| private static readonly string ContainerId = "Items"; | ||
|
|
||
| [TestMethod] | ||
| public async Task Main() | ||
| { | ||
| try | ||
| { | ||
| Console.WriteLine("Beginning operations..."); | ||
|
|
||
| using CosmosClient client = new( | ||
| accountEndpoint: EndpointUrl, | ||
| authKeyOrResourceToken: PrimaryKey, | ||
| new CosmosClientOptions { ConnectionMode = ConnectionMode.Gateway } | ||
| ); | ||
|
|
||
| // Create database if it doesn't exist | ||
| Database database = await client.CreateDatabaseIfNotExistsAsync(DatabaseId); | ||
| Console.WriteLine($"Created database: {database.Id}"); | ||
|
|
||
| // Create container if it doesn't exist | ||
| Container container = await database.CreateContainerIfNotExistsAsync(ContainerId, "/id"); | ||
| Console.WriteLine($"Created container: {container.Id}"); | ||
|
|
||
| // Create a sample item | ||
| TodoItem todoItem = new TodoItem | ||
| { | ||
| id = Guid.NewGuid().ToString(), | ||
| Title = "Learn Cosmos DB", | ||
| IsComplete = false | ||
| }; | ||
|
|
||
| // Add the item | ||
| await container.CreateItemAsync(todoItem); | ||
| Console.WriteLine($"Created item: {todoItem.id}"); | ||
|
|
||
| string[] someStringArray = ["Learn Cosmos DB"]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| IOrderedQueryable<TodoItem> queryable = container.GetItemLinqQueryable<TodoItem>(); | ||
| IQueryable<TodoItem> query = queryable.Where(item => someStringArray.Contains(item.Title)); | ||
|
|
||
| string querytest = query.ToQueryDefinition().QueryText; | ||
| Console.WriteLine($"Generated SQL Query: {querytest}"); | ||
|
|
||
| using FeedIterator<TodoItem> feed = query.ToFeedIterator(); | ||
|
|
||
| while (feed.HasMoreResults) | ||
| { | ||
| foreach(TodoItem item in await feed.ReadNextAsync()) | ||
| { | ||
| Console.WriteLine($"Item: {item.id}"); | ||
| } | ||
| } | ||
| } | ||
| catch (CosmosException cosmosEx) | ||
| { | ||
| Console.WriteLine($"Cosmos DB Error: {cosmosEx.Message}"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Error: {ex}"); | ||
| } | ||
| } | ||
|
|
||
| private class TodoItem | ||
| { | ||
| #pragma warning disable IDE1006 // Naming Styles | ||
| public string id { get; set; } | ||
| #pragma warning restore IDE1006 // Naming Styles | ||
| public string Title { get; set; } | ||
| public bool IsComplete { get; set; } | ||
| public string[] ArrayField { get; set; } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1286,7 +1286,10 @@ public void TestStringFunctions() | |
| Func<bool, IQueryable<DataObject>> getQuery = this.CreateDataTestStringFunctions(); | ||
|
|
||
| List<LinqTestInput> inputs = new List<LinqTestInput> | ||
| { | ||
| { | ||
| //// Memory Span Conversion | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert. |
||
| //new LinqTestInput("Contains in constant list", b => getQuery(b).Select(doc => constantList.Contains(doc.StringField))), | ||
| //new LinqTestInput("Contains in constant array", b => getQuery(b).Select(doc => constantArray.Contains(doc.StringField))), | ||
| // Concat | ||
| new LinqTestInput("Concat 2", b => getQuery(b).Select(doc => string.Concat(doc.StringField, "str"))), | ||
| new LinqTestInput("Concat 3", b => getQuery(b).Select(doc => string.Concat(doc.StringField, "str1", "str2"))), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,15 +3,15 @@ | |
| <IsTestProject>true</IsTestProject> | ||
| <TreatWarningsAsErrors>true</TreatWarningsAsErrors> | ||
| <Platform>AnyCPU</Platform> | ||
| <TargetFramework>net6.0</TargetFramework> | ||
| <TargetFramework>net9.0</TargetFramework> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert. |
||
| <IsPackable>false</IsPackable> | ||
| <GenerateAssemblyInfo>false</GenerateAssemblyInfo> | ||
| <RootNamespace>Microsoft.Azure.Cosmos</RootNamespace> | ||
| <AssemblyName>Microsoft.Azure.Cosmos.EmulatorTests</AssemblyName> | ||
| <IsEmulatorTest>true</IsEmulatorTest> | ||
| <EmulatorFlavor>master</EmulatorFlavor> | ||
| <DisableCopyEmulator>True</DisableCopyEmulator> | ||
| <LangVersion>$(LangVersion)</LangVersion> | ||
| <LangVersion>preview</LangVersion> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert. |
||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we always guarantee this? Either we should assert before or do a check.