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

Refactored queries to make supporting SQL Server Limit and Skip Logic easier. #592

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

wynan
Copy link
Contributor

@wynan wynan commented Mar 19, 2024

Looking to support SQL Server in the future and noticed changes were required for Limit and Skip Locking.

Some example SQL Server queries to support this:
Select Batch
SELECT TOP ({{batchSize}}) {{allFields}} FROM {{table}} WITH (UPDLOCK, ROWLOCK, READPAST) WHERE nextAttemptTime < ? AND blocked = 0 AND processed = 0
Select Version
SELECT version FROM TXNO_VERSION WITH (UPDLOCK, ROWLOCK, READPAST)
Delete Expired
DELETE TOP ({{batchSize}}) FROM {{table}} WHERE nextAttemptTime < ? AND processed = 1 AND blocked = 0

Among others.

This would be the first of a series of changes to support SQL Server.

@badgerwithagun
Copy link
Member

Thanks @wynan!

This looks pretty sensible. Though I suspect at this point that there's little point keeping the DefaultDialect; we may as well just make these individual classes. Maybe a little bit of inheritance for different versions of MySQL...

I'd probably consider pre-preparing the SQL strings too (e.g. interpolating ALL_FIELDS) on construction rather than doing it every time.

I can't see any reason to not approve this PR though assuming all the tests pass.

@wynan
Copy link
Contributor Author

wynan commented Mar 20, 2024

Totally agree about the DefaultDialect and the pre-preparing the SQL strings as well. Wanted to be careful about making too drastic a change too quickly. I'll look at those in future PRs.

@wynan
Copy link
Contributor Author

wynan commented Mar 26, 2024

Just wanted to confirm you don't require any changes for this PR and the comments can be addressed in future PRs?

@badgerwithagun
Copy link
Member

Thanks for reminder. Last checks now

@badgerwithagun badgerwithagun merged commit 83a187b into gruelbox:master Mar 26, 2024
17 checks passed
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.

3 participants