Skip to content

Add support for DATA_COMPRESSION and SORT_IN_TEMPDB SQL Server index options#30831

Merged
bricelam merged 2 commits intodotnet:release/8.0from
hmajerus:SqlServerIndexOptions
Sep 14, 2023
Merged

Add support for DATA_COMPRESSION and SORT_IN_TEMPDB SQL Server index options#30831
bricelam merged 2 commits intodotnet:release/8.0from
hmajerus:SqlServerIndexOptions

Conversation

@hmajerus
Copy link
Contributor

@hmajerus hmajerus commented May 5, 2023

Closes #30408

Please check whether the PR fulfills these requirements

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a (loosely) 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

@hmajerus
Copy link
Contributor Author

hmajerus commented May 5, 2023

@dotnet-policy-service agree company="Buildertrend"

@ajcvickers ajcvickers self-assigned this May 31, 2023
@hmajerus
Copy link
Contributor Author

hmajerus commented Jun 13, 2023

@ajcvickers and/or @bricelam Sorry for the ping, but let me know if there is anything I can provide or do to make this PR more manageable to review. I have the time to account for any recommendations you may have.

bricelam
bricelam suggested changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

This design looks good to me.

I want @AndriySvyryd to review the metadata changes.

You're also missing the logic to put this in the migrations model snapshot. Without it, you'll keep getting drop-creates every migration. I think that logic lives here:

efcore/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs

Lines 335 to 346 in afef2e5

protected override MethodCallCodeFragment? GenerateFluentApi(IIndex index, IAnnotation annotation)
=> annotation.Name switch
{
SqlServerAnnotationNames.Clustered => (bool)annotation.Value! == false
? new MethodCallCodeFragment(IndexIsClusteredMethodInfo, false)
: new MethodCallCodeFragment(IndexIsClusteredMethodInfo),
SqlServerAnnotationNames.Include => new MethodCallCodeFragment(IndexIncludePropertiesMethodInfo, annotation.Value),
SqlServerAnnotationNames.FillFactor => new MethodCallCodeFragment(IndexHasFillFactorMethodInfo, annotation.Value),
_ => null
};

@bricelam bricelam requested a review from AndriySvyryd August 24, 2023 16:58
@bricelam bricelam changed the base branch from main to release/8.0 August 24, 2023 16:58
@hmajerus
Copy link
Contributor Author

hmajerus commented Aug 24, 2023

Cool, I'm busy here at the end of the week, but will get to fixing that next week.

@bricelam
Copy link
Contributor

bricelam commented Aug 24, 2023

No rush, sorry it took us so long to get to

@hmajerus
Add IsSortedInTempDb and UseDataCompression to snapshots with SqlServ…
25f512b 
…erIndexBuilderExtensions. Add tests to verify snapshot for IsSortedInTempDb, UseDataCompression, and FillFactor.
@hmajerus
Copy link
Contributor Author

hmajerus commented Sep 13, 2023

Updated to use SqlServerIndexBuilderExtensions for DataCompression and SortInTempDb in snapshots.

Want to point out that without that update the snapshots would still contain something that looks like this:

b.HasIndex("Id").HasAnnotation("SqlServer:DataCompression", DataCompressionType.Row);

Which will prevent those drop-creates. This is currently the case for the IsCreatedOnline Index option.

This update makes the snapshot look like the following, which I imagine is preferred to the one above anyway.

SqlServerIndexBuilderExtensions.UseDataCompression(b.HasIndex("Id"), DataCompressionType.Row);

@bricelam bricelam merged commit be31c61 into dotnet:release/8.0 Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndriySvyryd AndriySvyryd AndriySvyryd approved these changes

+1 more reviewer

@bricelam bricelam bricelam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bricelam bricelam

Labels

community-contribution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SQL Server Index options SortInTempDB and DataCompression

4 participants

@hmajerus @bricelam @AndriySvyryd @ajcvickers