-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Reverse engineering of Dynamics CRM TDS endpoint and Synapse Serverless SQL Pool #29122
Conversation
…Synapse Serverless SQL Pool SKUs for query only operations Index query breaks on TDS endpoint, but we can just skip it sys.views does not exist on TDS endpoint No temporal, memory optimized or sequences in these engines Rephrase of FK query due to lack of schema functions on TDS endpoint Smoke tested against both engines with a CRM schema fixes dotnet#29121
src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs
Outdated
Show resolved
Hide resolved
=> _compatibilityLevel >= 110 && _engineEdition != 6; | ||
=> _compatibilityLevel >= 110 && (_engineEdition is not 6 and not 11 and not 1000); | ||
|
||
private bool SupportsViewsAndIndexes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at all these functions and their usages, may be we should cache them in private field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_compatibilityLevel and _engineEdition are already private fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could cache the function outputs (i.e. _supportsTemporalTable), though the perf impact here would probably be negligible (it's scaffolding after all).
@smitpatel @roji Any more I need to do here - or are you just busy? |
@ErikEJ sorry this didn't receive any attention. I'll likely be very busy until 7.0 is released, if nobody else on the team picks this up until then I'll revisit this. |
@roji Thanks, as suspected you are very busy - no rush... 😄 |
@roji are we there yet? 😄 |
@ErikEJ almost 🤣 Am slowly getting control of my backlog, promise to review in the next couple of days, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, see small comments.
=> _compatibilityLevel >= 110 && _engineEdition != 6; | ||
=> _compatibilityLevel >= 110 && (_engineEdition is not 6 and not 11 and not 1000); | ||
|
||
private bool SupportsViewsAndIndexes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could cache the function outputs (i.e. _supportsTemporalTable), though the perf impact here would probably be negligible (it's scaffolding after all).
src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs
Show resolved
Hide resolved
Are all comments properly addressed now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @ErikEJ! You forgot one SupportsViewsAndIndexes (does not compile), will merge after that's fixed.
Build break fixed |
Thanks @ErikEJ! |
…Synapse Serverless SQL Pool SKUs for query only operations
Index query breaks on TDS endpoint, but we can just skip it
sys.views does not exist on TDS endpoint
No temporal, memory optimized or sequences in these engines
Rephrase of FK query due to lack of schema functions on TDS endpoint
Smoke tested against both engines with a CRM schema
fixes #29121