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

ability to clean WITH clause matching CONTAIN process #17563

Open
wants to merge 4 commits into
base: 5.x
Choose a base branch
from

Conversation

mikeweb85
Copy link

Added NULL condition to overwrite and clear SQL WITH clause matching the CONTAIN process.

@dereuromark dereuromark added this to the 5.0.5 milestone Jan 27, 2024
@dereuromark
Copy link
Member

Ideally this should come with a test case.

@markstory markstory modified the milestones: 5.0.5, 5.0.6 Jan 28, 2024
@othercorey othercorey requested review from ndm2 and removed request for othercorey January 29, 2024 07:44
Comment on lines +405 to +407
if ($cte) {
$this->_parts['with'][] = $cte;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if a no-op like with(null) makes sense, but other query builder methods with override support allow similar no-ops.

@ndm2
Copy link
Contributor

ndm2 commented Jan 29, 2024

Technically this would need to go into 6.x, as the signature change is not backwards compatible. If it's considered low risk, then maybe it could go into 5.next.

@markstory
Copy link
Member

markstory commented Jan 31, 2024

Technically this would need to go into 6.x, as the signature change is not backwards compatible.

Why isn't it though? All call sites that were valid before are still valid. The addition is new semantic meaning to a newly supported type. We don't have a clear example of 'adding nullability' in our backwards compatibility guide but perhaps it is time we decide on that.

I agree on this going to 5.next though as it isn't a bugfix.

Currently the test is failing, but perhaps I don't understand the use
case well enough.
@markstory
Copy link
Member

I've added a test, and I'm not sure I understand the workflow this change would enable. Any examples you could share?

@mikeweb85
Copy link
Author

sorry, been I'll. yes, I have an example that prompted this tweak. I'll drop that here in the morning.

@ndm2
Copy link
Contributor

ndm2 commented Jan 31, 2024

Technically this would need to go into 6.x, as the signature change is not backwards compatible.

Why isn't it though? All call sites that were valid before are still valid. The addition is new semantic meaning to a newly supported type. We don't have a clear example of 'adding nullability' in our backwards compatibility guide but perhaps it is time we decide on that.

I agree on this going to 5.next though as it isn't a bugfix.

Now that we have actually typed arguments, adding a type (null or otherwise) would break all overridden methods, and so far breaking a public API in this was always considered not BC?

@mikeweb85
Copy link
Author

here is an example for where/why resetting the CTE was useful. Existing paginator has memory issues with large MSSQL datasets.
image

@markstory markstory modified the milestones: 5.0.6, 5.0.7 Mar 9, 2024
@markstory markstory modified the milestones: 5.0.7, 5.0.8 Apr 6, 2024
@markstory markstory modified the milestones: 5.0.8, 5.0.9 May 11, 2024
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.

None yet

4 participants