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

Don't duplicate query filter parameters #29422

Merged
merged 2 commits into from
Mar 27, 2023
Merged

Conversation

stevendarby
Copy link
Contributor

  • Checks for an already added parameter with an equal expression and uses that parameter name without adding another
  • Comparing the parameter to a slightly different type still produces a separate parameter in SQL Server as test shows

Fixes #24476

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

@stevendarby
Copy link
Contributor Author

If there's anything I need to do to move this along, let me know. If it's just that other things are taking a priority, that's fine!

Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Bad implementation

@stevendarby
Copy link
Contributor Author

stevendarby commented Nov 9, 2022

Is the failed build an environmental issue? Looks like it cancelled after waiting 3 hours. Not sure if it can be retried without another push.

Edit: failed again, so might be due to a change in my PR after all. All builds ok locally so a bit stumped.
Edit 2: it's now working 🤷

@stevendarby
Copy link
Contributor Author

@smitpatel thanks for initial review, changes are much simpler now.

@stevendarby
Copy link
Contributor Author

@maumar this post here may be useful for understanding how I arrived at these changes: #29422 (comment) - thanks

@stevendarby
Copy link
Contributor Author

@maumar any feedback?

@stevendarby
Copy link
Contributor Author

@ajcvickers is this likely to get reviewed again anytime soon? After @smitpatel's feedback, the changes are quite small and I'm fairly confident in them.

@ajcvickers
Copy link
Member

@stevendarby We will get to it when we can, but since Smit is no longer on the team it is taking some time.

@stevendarby
Copy link
Contributor Author

@ajcvickers ah I see, thanks for the update

@stevendarby
Copy link
Contributor Author

@maumar sorry to pester, but now the 8.0 previews have begun I was just thinking it'd be great to get it into one; and just aware that another 5 months will be too late. Is there any chance of a review over the next few weeks?

@maumar
Copy link
Contributor

maumar commented Mar 21, 2023

@stevendarby I'm planning to take a look at it this week. Apologies for the long delay.

@maumar maumar dismissed smitpatel’s stale review March 27, 2023 18:33

feedback addressed

@maumar maumar merged commit f50cc97 into dotnet:main Mar 27, 2023
@maumar
Copy link
Contributor

maumar commented Mar 27, 2023

@stevendarby thank you for the contribution!

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

Successfully merging this pull request may close these issues.

Global query filters produce too many parameters
4 participants