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-4883: Support SkipWhile and TakeWhile methods in LINQ. #1613

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rstam
Copy link
Contributor

@rstam rstam commented Feb 18, 2025

No description provided.

@rstam rstam requested a review from a team as a code owner February 18, 2025 02:11
Copy link
Member

@damieng damieng left a comment

Choose a reason for hiding this comment

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

Just a couple of suggestions, nothing blocking. Nice work.

var method = expression.Method;
var arguments = expression.Arguments;

if (method.IsOneOf(__skipWhileOrTakeWhileMethods))
Copy link
Member

Choose a reason for hiding this comment

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

I know this is the pattern used in other expression translators but I wonder if we should switch to a negative if and throw the ExpressionNotSupportedException here to avoid the indenting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes there's more than one if statement. I'm fine with the current pattern.

var valuePredicateField = AstExpression.GetField(valueVar, "predicate");
var valueCountField = AstExpression.GetField(valueVar, "count");

var reduceAst = AstExpression.Reduce(
Copy link
Member

Choose a reason for hiding this comment

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

What's the min version/wire version required for this reduce/while/predicate MQL construct? Do we want to throw if it's not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The server documentation is very unhelpful for determining when a feature was first introduced. Sometimes we just have to let the patch build run and see if something fails on older servers.

Based on this patch build looks like all the MQL operators used in this PR are supported on all the servers we still support.

Also, we never throw when something is not supported. We let the server do that.

var whileBinding = AstExpression.VarBinding(whileVar, reduceAst);
var whileCountField = AstExpression.GetField(whileVar, "count");

var sliceAst = method switch
Copy link
Member

Choose a reason for hiding this comment

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

I understand the slicing here and the idea of a reduce above but when the predicate stops matching how does it terminate/stop counting ones that match after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The predicate field of the accumulator value starts out true and stays true as long as the predicate keeps returning true. The count field indicates how many times the predicate has returned true so far.

Once the predicate returns false the accumulator predicate field is set to false, and once that happens, we never execute the predicate or increment the count again.

The first case of the $switch is where this happens. When the predicate field is false we just keep returning { predicate : false, count : n } over and over until the array is exhausted. There is no way in MQL to short circuit the reduction to ignore the rest of the array.

Copy link
Contributor

@adelinowona adelinowona left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sanych-sun sanych-sun left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants