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.

Sign up for GitHub

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

Jump to bottom

Custom aggregate translations #28110

Merged
1 commit merged into from
Jul 9, 2022
Merged

Custom aggregate translations #28110

1 commit merged into from
Jul 9, 2022

Conversation

roji
Copy link
Member

@roji roji commented May 26, 2022 •
edited
Loading

@smitpatel here's some work on custom aggregates:

  • string.Join (SQL Server and SQLite)
  • string.Concat (SQL Server and SQLite)
  • Standard deviation and variance (SQL Server)

I recommend reviewing it commit-by-commit. I went ahead and introduced SqlServerSqlFunctionExpression for SQL Server's ordering (last commit).

Closes #2981
Closes #28104

@roji roji requested a review from smitpatel May 26, 2022 15:54
roji
roji commented May 26, 2022
View reviewed changes
src/EFCore.Relational/Query/SqlExpressionFactory.cs Outdated
elseResult: null);
}

if (enumerableExpression.IsDistinct)
Copy link
Member Author

Choose a reason for hiding this comment

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

@smitpatel I think we may be able to move the distinct to SqlFunctionExpression, like I've done in https://github.com/npgsql/efcore.pg/pull/2383/files#diff-9244978be2a10140c59eb33bab6b774599d3ff3e47a0d639b5293e82bca8c39fR21 for PG. This would allow us to remove DistinctExpression.

Copy link
Contributor

Choose a reason for hiding this comment

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

If Distinct only occurs in the context of SqlFunction we can. Given we have already shipped it, I am fine keeping it too (even though incorrect). And break it when we have a better place to put it.

Copy link
Member Author

Choose a reason for hiding this comment

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

If Distinct only occurs in the context of SqlFunction we can.

I think that's true - at least as far as I'm aware... Up to you if we should do this or not.

@smitpatel
Copy link
Contributor

string.Join/Concat are fine that they are not our methods. For others we need better design in terms of how to invoke them at top-level.

@roji
Copy link
Member Author

roji commented May 26, 2022

Hmm, tests are failing with:

Microsoft.Data.SqlClient.SqlException : 'STRING_AGG' is not a recognized built-in function name.

According to the docs, STRING_AGG, is available from SQL Server 2017 and up - any chance we're using an older version???

smitpatel
smitpatel reviewed May 26, 2022
View reviewed changes
src/EFCore.SqlServer/Query/Internal/SqlServerSqlFunctionExpression.cs Outdated
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public class SqlServerSqlFunctionExpression : SqlFunctionExpression, IEquatable<SqlServerSqlFunctionExpression>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks little problematic to add a facet on SqlFunctionExpression which is not about all the functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, isn't the ordering a facet of aggregate functions? Or did you see this as a separate wrapping expression node (which would only be legal over an aggregate expression)?

Note that in the PG case, the ORDER BY comes before the closing parenthesis, making it even more strongly part of the function expression (at least from an SQL generation perspective).

Let me know if you'd like it some other way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw it as latter. Wondering if we should have a separate custom expression for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Up to you... Some thoughts:

  • I think if we have a node type that can only have one node type as an operand, that's a good case for just integrating it inside that operand.
  • SelectExpression also has orderings applied inside, so it would be consistent with that.
  • We can call the new SQL Server expression SqlServerAggregateFunctionExpression, to make it specific for aggregate use. That separates things more cleanly (the ordering would only go on aggregate function, which is how it is), and 's maybe better in general since there's no reason to do the work to support e.g. niladic (no niladic aggregate functions).
  • In any case, all this is internal to SQL Server so we can always change it if we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Decided to create a new custom expression for this. If we find a good abstraction across provider then we can change it later as this is all pubternal to SqlServer anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, just to be sure: you want a SqlServerAggregateFunctionExpression where we'd have the ordering, right? Or a separate OrderingExpression which would wrap a SqlFunctionExpression to add ordering around it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The former. Just a custom expression representing the aggregate with ordering on SqlServer. No wrapping.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I've basically renamed the existing SqlServerSqlFunctionExpression to SqlServerAggregateFunctionExpression: it extends SqlFunctionExpression and adds orderings to it. We only use it for cases where we actually need the orderings, which is only STRING_AGG at this point (for all the others ordering is irrelevant).

Let me know if this is what you want, I can make adjustments.

@smitpatel
Copy link
Contributor

According to the docs, STRING_AGG, is available from SQL Server 2017 and up - any chance we're using an older version???

Windows could be using 2016. You may need to add conditional theory.

Helix has ubuntu config with 2019 installed which would be able to validate the changes.

@roji
Copy link
Member Author

roji commented May 26, 2022

Windows could be using 2016. You may need to add conditional theory.

Would be nice if one day we could get a newer version :/

I'll look at adding a SqlServerCondition (maybe just directly for the SQL Server version).

@roji roji mentioned this pull request Jun 5, 2022
Closed
4 tasks
@roji
Copy link
Member Author

roji commented Jun 18, 2022

@smitpatel now that we've excluded top-level aggregates (#28264), I think we can come back to this.

@roji roji force-pushed the AggregateStuff branch 2 times, most recently from 7d3dce6 to d54a174 Compare June 18, 2022 17:11
@roji roji requested a review from smitpatel June 21, 2022 22:03
@roji
Copy link
Member Author

roji commented Jun 22, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@roji roji requested a review from smitpatel July 4, 2022 10:41
This was referenced Jul 6, 2022
Closed
Closed
@roji roji requested a review from smitpatel July 9, 2022 06:48
@roji
Some custom aggregate translations
578fd86 
* string.Join (SQL Server and SQLite)
* string.Concat (SQL Server and SQLite)
* Standard deviation and variance (SQL Server)

Closes dotnet#2981
Closes dotnet#28104
@roji roji force-pushed the AggregateStuff branch from 7b0045b to 578fd86 Compare July 9, 2022 06:58
@roji roji added the auto-merge label Jul 9, 2022
@ghost
Copy link

ghost commented Jul 9, 2022

Hello @roji!

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
Copy link

ghost commented Jul 9, 2022

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.

@ghost ghost merged commit 983112b into dotnet:main Jul 9, 2022
@roji roji deleted the AggregateStuff branch July 9, 2022 10:29
@ajcvickers ajcvickers mentioned this pull request Jul 21, 2022
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smitpatel smitpatel smitpatel approved these changes

Assignees
No one assigned
Labels
auto-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SQL Server: Translate statistics aggregate functions Translate string.Join and string.Concat
2 participants
@roji @smitpatel