Copilot commented on code in PR #7933:
URL: https://github.com/apache/ignite-3/pull/7933#discussion_r3035599059
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Linq/IgniteQueryParser.cs:
##########
@@ -78,16 +86,79 @@ private static CompoundExpressionTreeProcessor
CreateCompoundProcessor(
return new CompoundExpressionTreeProcessor(
new IExpressionTreeProcessor[]
{
- new PartialEvaluatingExpressionTreeProcessor(new
NullEvaluatableExpressionFilter()),
+ new PartialEvaluatingExpressionTreeProcessor(new
IgniteEvaluatableExpressionFilter()),
new TransformingExpressionTreeProcessor(transformationProvider)
});
}
/// <summary>
- /// Empty implementation of IEvaluatableExpressionFilter.
+ /// Implementation of IEvaluatableExpressionFilter.
/// </summary>
- private sealed class NullEvaluatableExpressionFilter :
EvaluatableExpressionFilterBase
+ private sealed class IgniteEvaluatableExpressionFilter :
EvaluatableExpressionFilterBase
{
- // No-op.
+ // Ignores implicit ReadOnlySpan conversion to support C# 14 first
class span Contains.
+ public override bool IsEvaluatableMethodCall(MethodCallExpression node)
+ {
+ ArgumentNullException.ThrowIfNull(node);
+ if
(MemoryExtensionsContainsExpressionTransformer.IsSpanImplicitConversion(node))
+ {
+ return false;
+ }
+
+ return base.IsEvaluatableMethodCall(node);
+ }
+ }
+
+ /// <summary>
+ /// Implementation of IExpressionTransformer handling C# 14 first class
span Contains.
+ /// </summary>
+ private sealed class MemoryExtensionsContainsExpressionTransformer :
IExpressionTransformer<MethodCallExpression>
+ {
+ private static readonly MethodInfo SourceMethodInfo =
typeof(MemoryExtensions)
+ .GetMethod(nameof(MemoryExtensions.Contains), [
+
typeof(ReadOnlySpan<>).MakeGenericType(Type.MakeGenericMethodParameter(0)),
+ Type.MakeGenericMethodParameter(0)
+ ])!;
+
+ private static readonly MethodInfo TargetMethodInfo =
typeof(Enumerable)
+ .GetMethod(nameof(Enumerable.Contains), [
+
typeof(IEnumerable<>).MakeGenericType(Type.MakeGenericMethodParameter(0)),
+ Type.MakeGenericMethodParameter(0)
+ ])!;
Review Comment:
`TargetMethodInfo` is obtained via reflection
(`typeof(Enumerable).GetMethod(...)`) and then invoked via `MakeGenericMethod`.
Since `Apache.Ignite` is marked trimmable, this kind of reflection can break in
trimmed apps if the linker removes the `Enumerable.Contains` overload. Prefer a
direct method reference (e.g., obtain `MethodInfo` from a closed generic
delegate and call `GetGenericMethodDefinition()`), or add the necessary
trimming annotations/dependencies to ensure the method is preserved.
```suggestion
private static readonly MethodInfo TargetMethodInfo =
((Func<IEnumerable<int>, int,
bool>)Enumerable.Contains).Method.GetGenericMethodDefinition();
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Linq/IgniteQueryParser.cs:
##########
@@ -78,16 +86,79 @@ private static CompoundExpressionTreeProcessor
CreateCompoundProcessor(
return new CompoundExpressionTreeProcessor(
new IExpressionTreeProcessor[]
{
- new PartialEvaluatingExpressionTreeProcessor(new
NullEvaluatableExpressionFilter()),
+ new PartialEvaluatingExpressionTreeProcessor(new
IgniteEvaluatableExpressionFilter()),
new TransformingExpressionTreeProcessor(transformationProvider)
});
}
/// <summary>
- /// Empty implementation of IEvaluatableExpressionFilter.
+ /// Implementation of IEvaluatableExpressionFilter.
/// </summary>
- private sealed class NullEvaluatableExpressionFilter :
EvaluatableExpressionFilterBase
+ private sealed class IgniteEvaluatableExpressionFilter :
EvaluatableExpressionFilterBase
{
- // No-op.
+ // Ignores implicit ReadOnlySpan conversion to support C# 14 first
class span Contains.
+ public override bool IsEvaluatableMethodCall(MethodCallExpression node)
+ {
+ ArgumentNullException.ThrowIfNull(node);
+ if
(MemoryExtensionsContainsExpressionTransformer.IsSpanImplicitConversion(node))
+ {
+ return false;
+ }
+
+ return base.IsEvaluatableMethodCall(node);
+ }
+ }
+
+ /// <summary>
+ /// Implementation of IExpressionTransformer handling C# 14 first class
span Contains.
+ /// </summary>
+ private sealed class MemoryExtensionsContainsExpressionTransformer :
IExpressionTransformer<MethodCallExpression>
+ {
+ private static readonly MethodInfo SourceMethodInfo =
typeof(MemoryExtensions)
+ .GetMethod(nameof(MemoryExtensions.Contains), [
+
typeof(ReadOnlySpan<>).MakeGenericType(Type.MakeGenericMethodParameter(0)),
+ Type.MakeGenericMethodParameter(0)
+ ])!;
+
+ private static readonly MethodInfo TargetMethodInfo =
typeof(Enumerable)
+ .GetMethod(nameof(Enumerable.Contains), [
+
typeof(IEnumerable<>).MakeGenericType(Type.MakeGenericMethodParameter(0)),
+ Type.MakeGenericMethodParameter(0)
+ ])!;
+
+ public static bool
IsSpanImplicitConversion([NotNullWhen(true)]MethodCallExpression? node) =>
+ node?.Method is { IsSpecialName: true, Name: "op_Implicit" }
+ && node.Method.DeclaringType is { IsGenericType: true }
+ && node.Method.DeclaringType.GetGenericTypeDefinition() ==
typeof(ReadOnlySpan<>);
+
+ public Expression Transform(MethodCallExpression expression)
+ {
+ if (expression.Method.IsConstructedGenericMethod &&
expression.Method.GetGenericMethodDefinition() == SourceMethodInfo)
+ {
+ var genericType = expression.Method.GetGenericArguments()[0];
+
+ var enumerableParameter = expression.Arguments[0] as
MethodCallExpression;
+ if (!IsSpanImplicitConversion(enumerableParameter))
+ {
+ throw new NotSupportedException("Was not able to process
parameter for MemoryExtensions.Contains, " +
+ "expected implicit
conversion, got: " + expression.Arguments[0]);
+ }
+
+ var argument = enumerableParameter.Arguments[0]!;
+
+ var targetProp = expression.Arguments[1];
+
+ var target = TargetMethodInfo.MakeGenericMethod(genericType);
+ return Expression.Call(target, argument, targetProp);
+ }
+
+ return expression;
+ }
+
+#pragma warning disable CA1819
+#pragma warning disable SA1201
+ public ExpressionType[] SupportedExpressionTypes =>
[ExpressionType.Call];
+#pragma warning restore SA1201
+#pragma warning restore CA1819
Review Comment:
The `SupportedExpressionTypes` property returns a new array each access and
adds pragma suppressions (CA1819/SA1201) inline. Consider switching to a
`static readonly ExpressionType[]` field and/or reordering members to avoid
StyleCop suppressions and repeated allocations.
##########
modules/platforms/dotnet/Apache.Ignite.Tests/Linq/LinqSqlGenerationTests.cs:
##########
@@ -158,6 +158,14 @@ public void TestSelectOrderByOffsetLimit() =>
.Take(3)
.ToList());
+ [Test]
+ public void TestContains() =>
+ AssertSql("select (_T0.KEY IN (?, ?)) from PUBLIC.TBL1 as _T0", q =>
+ {
+ var keys = new long[] { 4, 2 };
+ return q.Select(x => keys.Contains(x.Key)).ToList();
+ });
Review Comment:
This test uses `keys.Contains(x.Key)` under the repo-wide `LangVersion=12`,
where the call is expected to bind to `Enumerable.Contains` (not
`MemoryExtensions.Contains`). As a result, it likely doesn't exercise the new
`MemoryExtensionsContainsExpressionTransformer` / span implicit-conversion
filter. Consider adding a test that explicitly builds (or forces) a
`MemoryExtensions.Contains(ReadOnlySpan<T>, T)` call in the expression tree so
the new code path is covered.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Linq/IgniteQueryParser.cs:
##########
@@ -17,8 +17,15 @@
namespace Apache.Ignite.Internal.Linq;
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
+using System.Linq;
+using System.Linq.Expressions;
+using System.Reflection;
using System.Threading;
using Dml;
+using Remotion.Linq.Clauses.Expressions;
Review Comment:
`using Remotion.Linq.Clauses.Expressions;` appears to be unused in this
file. With `TreatWarningsAsErrors=true`, this will fail the build with CS8019;
please remove the unnecessary using directive.
```suggestion
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Linq/IgniteQueryParser.cs:
##########
@@ -78,16 +86,79 @@ private static CompoundExpressionTreeProcessor
CreateCompoundProcessor(
return new CompoundExpressionTreeProcessor(
new IExpressionTreeProcessor[]
{
- new PartialEvaluatingExpressionTreeProcessor(new
NullEvaluatableExpressionFilter()),
+ new PartialEvaluatingExpressionTreeProcessor(new
IgniteEvaluatableExpressionFilter()),
new TransformingExpressionTreeProcessor(transformationProvider)
});
}
/// <summary>
- /// Empty implementation of IEvaluatableExpressionFilter.
+ /// Implementation of IEvaluatableExpressionFilter.
/// </summary>
- private sealed class NullEvaluatableExpressionFilter :
EvaluatableExpressionFilterBase
+ private sealed class IgniteEvaluatableExpressionFilter :
EvaluatableExpressionFilterBase
{
- // No-op.
+ // Ignores implicit ReadOnlySpan conversion to support C# 14 first
class span Contains.
+ public override bool IsEvaluatableMethodCall(MethodCallExpression node)
+ {
+ ArgumentNullException.ThrowIfNull(node);
+ if
(MemoryExtensionsContainsExpressionTransformer.IsSpanImplicitConversion(node))
+ {
+ return false;
+ }
+
+ return base.IsEvaluatableMethodCall(node);
+ }
+ }
+
+ /// <summary>
+ /// Implementation of IExpressionTransformer handling C# 14 first class
span Contains.
+ /// </summary>
+ private sealed class MemoryExtensionsContainsExpressionTransformer :
IExpressionTransformer<MethodCallExpression>
+ {
+ private static readonly MethodInfo SourceMethodInfo =
typeof(MemoryExtensions)
+ .GetMethod(nameof(MemoryExtensions.Contains), [
+
typeof(ReadOnlySpan<>).MakeGenericType(Type.MakeGenericMethodParameter(0)),
+ Type.MakeGenericMethodParameter(0)
+ ])!;
+
+ private static readonly MethodInfo TargetMethodInfo =
typeof(Enumerable)
+ .GetMethod(nameof(Enumerable.Contains), [
+
typeof(IEnumerable<>).MakeGenericType(Type.MakeGenericMethodParameter(0)),
+ Type.MakeGenericMethodParameter(0)
+ ])!;
+
+ public static bool
IsSpanImplicitConversion([NotNullWhen(true)]MethodCallExpression? node) =>
+ node?.Method is { IsSpecialName: true, Name: "op_Implicit" }
+ && node.Method.DeclaringType is { IsGenericType: true }
+ && node.Method.DeclaringType.GetGenericTypeDefinition() ==
typeof(ReadOnlySpan<>);
+
+ public Expression Transform(MethodCallExpression expression)
+ {
+ if (expression.Method.IsConstructedGenericMethod &&
expression.Method.GetGenericMethodDefinition() == SourceMethodInfo)
+ {
+ var genericType = expression.Method.GetGenericArguments()[0];
+
+ var enumerableParameter = expression.Arguments[0] as
MethodCallExpression;
+ if (!IsSpanImplicitConversion(enumerableParameter))
+ {
+ throw new NotSupportedException("Was not able to process
parameter for MemoryExtensions.Contains, " +
+ "expected implicit
conversion, got: " + expression.Arguments[0]);
+ }
+
+ var argument = enumerableParameter.Arguments[0]!;
+
+ var targetProp = expression.Arguments[1];
+
+ var target = TargetMethodInfo.MakeGenericMethod(genericType);
+ return Expression.Call(target, argument, targetProp);
Review Comment:
`targetProp` is a misleading name here: the expression represents the value
being searched for (the second argument to `Contains`), not a property.
Renaming it to something like `value`/`item` would make the transformation
logic easier to follow.
```suggestion
var value = expression.Arguments[1];
var target = TargetMethodInfo.MakeGenericMethod(genericType);
return Expression.Call(target, argument, value);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
