Skip to content

Query: Add support for custom aggregate operators - #28092

Merged
1 commit merged into
mainfrom
smit/customoperators
May 25, 2022
Merged

Query: Add support for custom aggregate operators#28092
1 commit merged into
mainfrom
smit/customoperators

Conversation

@smitpatel

Copy link
Copy Markdown
Contributor

Resolves #22957

  • Introduce IAggregateMethodCallTranslator and family to translate aggregate methods
  • Currently we assume that either method instance or first argument (for static method) will be the one which can be of type enumerable, rest of the arguments if any are scalar.
  • Also refactor code in SqlTranslator.VisitMethodCall to avoid visiting grouping element source multiple times

Resolves #22957

- Introduce IAggregateMethodCallTranslator and family to translate aggregate methods
- Currently we assume that either method instance or first argument (for static method) will be the one which can be of type enumerable, rest of the arguments if any are scalar.
- Also refactor code in SqlTranslator.VisitMethodCall to avoid visiting grouping element source multiple times
@smitpatel
smitpatel requested a review from a team May 24, 2022 23:13
@ghost

ghost commented May 24, 2022

Copy link
Copy Markdown

Hello @smitpatel!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost

ghost commented May 24, 2022

Copy link
Copy Markdown

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

SqlExpression? sqlObject = null;
SqlExpression[] arguments;
var method = methodCallExpression.Method;
var arguments = methodCallExpression.Arguments;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@roji roji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome stuff!

Here are a few comments, the only important thing is the possibility to use custom aggregate functions in QueryableMethodTranslatingEV (e.g. at top-level).

I'll work on the PG side against this branch to make sure I'm not blocked by anything - will report results.

Comment thread src/EFCore.Relational/Query/IMethodCallTranslatorProvider.cs
Expression? result = null;
switch (methodCallExpression.Method.Name)
{
case nameof(Queryable.Average):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice to see all this gone from here 👍

Comment thread src/EFCore/Query/ICompiledQueryCacheKeyGenerator.cs
@ghost
ghost merged commit 97783f5 into main May 25, 2022
@ghost
ghost deleted the smit/customoperators branch May 25, 2022 16:43
@roji

roji commented May 25, 2022

Copy link
Copy Markdown
Member

Should remember to remove auto-merge... :(

@smitpatel

Copy link
Copy Markdown
Contributor Author

Ouch!

@roji

roji commented May 25, 2022

Copy link
Copy Markdown
Member

If you submit a fixup I'll approve, sorry...

This pull request was closed.
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.

Full support for custom provider aggregate functions

2 participants