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

add support distinct count #17825

Closed
wants to merge 2 commits into from
Closed

add support distinct count #17825

wants to merge 2 commits into from

Conversation

voronov-maxim
Copy link
Contributor

  • [x ] The code builds and tests pass (verified by our automated build checks)

fix issue #17376

@smitpatel
Copy link
Contributor

@voronov-maxim - Thank you for attempting to fix this issue but there are several issues in this PR.

  • Why do we need conversion to new array[].Distinct().Any in predicate for count. If that was helping any provider then we could but InMemory also just unwind it. So that conversion seems unnecessary.
  • The SelectDistictExpression has a lot of bugs in it. We would need a structure to represent Distinct Column. Probably it should be derived from ColumnExpression perhaps.
  • The processing of selector in TranslateCount seems too hacky to support this one case.
  • Select.Distinct.Aggregate is not limited to count but also apply to all aggregate operators. Min/Max of Distinct values would be same, but Sum/Average of Distinct value would be different from non-distinct.

Overall, it seems very specific approach to deal with one case. It should be more generalized approach to translate all such variation which are quite similar.

Due to aforementioned issues, before writing any code, we would need a good design on how do we want to translate this class of expression. Given this PR is not in a state where we can reach to final design through feedback I am closing this PR.

If you want to submit a fix for #17376, please submit a draft on approach you plan to implement. Also make sure it covers above questions/scenarios. Keep in mind that the issue is not marked as "good-first-issue". I believe the issue is fairly complex which requires changes in various parts of EF Core to work end to end. Also given it is a new feature and something which never translated in past, it is highly unlikely that this will get fixed in 3.1. I have removed the issue from milestone to bring to attention to management.

@smitpatel smitpatel closed this Sep 16, 2019
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.

2 participants