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

Remove GetAzureCompactScript from SqlDataProvider #2763

Merged

Conversation

ohine
Copy link
Contributor

@ohine ohine commented May 7, 2019

Summary

GetAzureCompactScript doesn't appear to be required any longer for azure compatibility and it adds quite a bit of overhead to every single sql query run through the Platform. This PR removes the check and removal of the following strings.

WITH\\s*\\([\\s\\S]*?((PAD_INDEX|ALLOW_ROW_LOCKS|ALLOW_PAGE_LOCKS)\\s*=\\s*(ON|OFF))+[\\s\\S]*?\\)

(TEXTIMAGE_)*ON\\s*\\[\\s*PRIMARY\\s*\\]

Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

I have verified that the items being matched against in these modifications are fully supported by Azure SQL. Therefore, since Azure SQL can support this, I don't see this as a breaking change.

Quick performance benchmarking of the processing necessary for this code shows that overall DNN Performance should be dramatically increased.

In other words...Looks good to me!

@mitchelsellers mitchelsellers added this to the 9.4.0 milestone May 7, 2019
Copy link
Contributor

@mtrutledge mtrutledge left a comment

Choose a reason for hiding this comment

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

Looks good

@mtrutledge mtrutledge merged commit 9899bd3 into dnnsoftware:development May 7, 2019
Copy link
Contributor

@david-poindexter david-poindexter left a comment

Choose a reason for hiding this comment

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

LGTM

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

5 participants