-
Notifications
You must be signed in to change notification settings - Fork 867
Context expressions - Add support for Scan and Query using LINQ #3872
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
base: development
Are you sure you want to change the base?
Context expressions - Add support for Scan and Query using LINQ #3872
Conversation
/// <remarks> | ||
/// Note: Conditions must be against non-key properties. | ||
/// </remarks> | ||
public ContextExpression Expression { get; set; } |
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.
We should be consistent also make the Expression
available on ScanConfig
. I see currently it is on DynamoDBOperationConfig
and QueryConfig
.
Also since you added the Scan
overloads that take in the expression seems we should also add the overloads for Query
.
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.
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.
Are you sure QueryFilter
is being ignored for Scan
? I see it being copied over in the ToDynamoDBOperationConfig
operation. If it is not being used that seems like a bug.
I saw we didn't have a list of arguments overload and I'm not sure why. It seems like from a discoverability point of view it makes sense to get the feature more upfront to users as an overload. Besides having yet another overload is there a downside?
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.
I've added a integration test for this scenario TestContext_ScanConfigFilter
in 'DataModelTests', and at this point it fails. To be sure my changes did not affect the existing behavior the same test I executed on main and also fails.
I will push a fix to this issue in this PR.
As for having both an overload and the config, apart form what should happen when both 'Expression's are provided I am not a big fan of having too many arguments in public interfaces and in Query
method case, for 'IEnumerable Query(object hashKeyValue, QueryOperator op, IEnumerable values, QueryConfig queryConfig);' will look like 'IEnumerable Query(object hashKeyValue, QueryOperator op, IEnumerable values, ContextExpression filterExpression, QueryConfig queryConfig);' and this just just for visibility.
sdk/src/Services/DynamoDBv2/Custom/DataModel/ContextExpression.cs
Outdated
Show resolved
Hide resolved
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.
We need to get the Native AOT fixed and tested.
if (value != null) | ||
{ | ||
// Use reflection to get the value of the member | ||
var memberInfo = value.GetType().GetMember(memberName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic).FirstOrDefault(); |
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.
Suspecting this would cause problems for Native AOT I made GetConstant
public and then create a console application with PublishAot
set to true.
using Amazon.DynamoDBv2.DataModel;
using System.Linq.Expressions;
var constExpr = Expression.Constant(42);
var result = ContextExpressionsUtils.GetConstant(constExpr);
When I ran dotnet publish
I got Native AOT trim warnings on this.
> dotnet publish
Restore complete (1.1s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
AWSSDK.Core.NetStandard net8.0 succeeded (3.3s) → C:\codebase\v4\aws-sdk-net-v4\sdk\src\Core\bin\Release\net8.0\AWSSDK.Core.dll
AWSSDK.DynamoDBv2.NetStandard net8.0 succeeded (2.8s) → C:\codebase\v4\aws-sdk-net-v4\sdk\src\Services\DynamoDBv2\bin\Release\net8.0\AWSSDK.DynamoDBv2.dll
DDBNativeAOTTest succeeded with 1 warning(s) (6.3s) → bin\Release\net8.0\win-x64\publish\
C:\codebase\v4\aws-sdk-net-v4\sdk\src\Services\DynamoDBv2\Custom\DataModel\ContextExpression.cs(167): Trim analysis warning IL2075: Amazon.DynamoDBv2.DataModel.ContextExpressionsUtils.GetConstantFromMember(MemberExpression): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.NonPublicConstructors', 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods', 'DynamicallyAccessedMemberTypes.PublicFields', 'DynamicallyAccessedMemberTypes.NonPublicFields', 'DynamicallyAccessedMemberTypes.PublicNestedTypes', 'DynamicallyAccessedMemberTypes.NonPublicNestedTypes', 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.NonPublicProperties', 'DynamicallyAccessedMemberTypes.PublicEvents', 'DynamicallyAccessedMemberTypes.NonPublicEvents' in call to 'System.Type.GetMember(String,BindingFlags)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
This is the tricky thing of Native AOT is the checks done in Roslyn are not as extensive compared to actually doing a dotnet publish
. Given this PR is doing quite a bit of .NET typesystem/reflection work we need to make sure we do some Native AOT compilation exercising the work.
In your next revision of the PR can you include in the PR description your sample console application that you compiled for Native AOT so we can tell what code paths have been exercised.
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.
I've reimplemented the GetConstant to no longer use reflection and method 'System.Object.GetType()'
console app used to test it is here: TestNativeConsole
/// <remarks> | ||
/// Note: Conditions must be against non-key properties. | ||
/// </remarks> | ||
public ContextExpression Expression { get; set; } |
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.
Are you sure QueryFilter
is being ignored for Scan
? I see it being copied over in the ToDynamoDBOperationConfig
operation. If it is not being used that seems like a bug.
I saw we didn't have a list of arguments overload and I'm not sure why. It seems like from a discoverability point of view it makes sense to get the feature more upfront to users as an overload. Besides having yet another overload is there a downside?
This pull request introduces native support for LINQ expression trees in the IDynamoDBContext API for ScanAsync() and QueryAsync().
Description
Implement support to automatically translate LINQ expression trees to DynamoDB Expression
Example translation:
u => u.Age > 21 && u.Status == "Active"
becomes a ExpressionStatement like: #C0 > :C0 AND #C1 = :C1
with appropriate ExpressionAttributeNames and ExpressionAttributeValues.
ContextExpression
that will be used to define filter LINQ expressions as follows:Add in LinqDdbExtensions methods to be used inside expression trees to generate DynamoDB Expression functions as follows:
Extend
IDynamoDBContext
with a new overloads for Scan API:QueryConfig
to allow settingExpressionFilter
asContextExpression
.Motivation and Context
Testing
unit and integration tests added

benchmarking between filters methods and LINQ expression tree ones
Screenshots (if appropriate)
Types of changes
Checklist
License