Skip to content
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

CSHARP-5463: Investigate whether calls to EnsureQueryableMethodHasNestedAsQueryableSource are actually needed. #1669

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rstam
Copy link
Contributor

@rstam rstam commented Apr 12, 2025

We should probably have a Zoom meeting to discuss this one.

@rstam rstam requested review from sanych-sun and adelinowona April 12, 2025 00:18
@rstam rstam requested a review from a team as a code owner April 12, 2025 00:18
@@ -27,7 +27,7 @@ public static void EnsureQueryableMethodHasNestedAsQueryableSource(MethodCallExp
if (expression.Method.DeclaringType == typeof(Queryable) &&
sourceTranslation.Serializer is not INestedAsQueryableSerializer)
{
throw new ExpressionNotSupportedException(expression, because: "source serializer is not a NestedAsQueryableSerializer");
throw new ExpressionNotSupportedException(expression, because: "source argument is an unsupported IQueryable type");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the message should be even more descriptive?

For example, it could include the type of the unsupported IQueryable...

var enumerable = new int[] { 1, 2, 3 };

var queryable = collection.AsQueryable()
.Select(x => enumerable.AsQueryable().Contains(1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the nested AsQueryable we've been supporting all along.

var nestedQueryable = new int[] { 1, 2, 3 }.AsQueryable();

var queryable = collection.AsQueryable()
.Select(x => nestedQueryable.Contains(1)); // PartialEvaluator turns this into Select(x => true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one doesn't trigger an error because the entire nested queryable expression is evaluated by the partial evaluator.

var nestedQueryable = new int[] { 1, 2, 3 }.AsQueryable();

var queryable = collection.AsQueryable()
.Select(x => nestedQueryable.Contains(x.Id));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one triggers an error because the translator doesn't know how to translate nestedAsQueryable.

@rstam rstam added the chore Label to hide PR from generated Release Notes label Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Label to hide PR from generated Release Notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant