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

Reveng: Only get NONCLUSTERED and CLUSTERED indexes from sys.indexes #3952

Closed
wants to merge 1 commit into from
Closed

Reveng: Only get NONCLUSTERED and CLUSTERED indexes from sys.indexes #3952

wants to merge 1 commit into from

Conversation

ErikEJ
Copy link
Contributor

@ErikEJ ErikEJ commented Dec 2, 2015

to avoid name being NULL and also ignore other irrelevant index types
https://msdn.microsoft.com/da-dk/library/ms173760.aspx
Fixes some of the issues reported in #3861

to avoid name being NULL and also ignore other irrelevant index types
https://msdn.microsoft.com/da-dk/library/ms173760.aspx
Fixes some issues reported in #3861
@lajones
Copy link
Contributor

lajones commented Dec 2, 2015

@ErikEJ Is this still needed after the fixes in #3934?

@natemcmaster
Copy link
Contributor

I don't believe we support SPATIAL index (and others) in Model. We should log about unsupported index types.

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Dec 3, 2015

@lajones Yes, in order to avoid spatial and other index types, and to clarify the intent of GetIndexes

@lajones
Copy link
Contributor

lajones commented Dec 3, 2015

@ErikEJ I'm with @natemcmaster I think it would be better to bring them back in the query and issue a warning message along the lines of "Index [{schemaName}].[{tableName}].[{indexName}] has type {typeDescription}. We only support CLUSTERED and NONCLUSTERED. Skipping index." (similar to what we already do for columns with types we don't understand). Note: we already bring back the type_desc column in the query so that should help.

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Dec 4, 2015

@lajones That would also work, yes, but just looking at the type_desc seems quite brittle (type_desc could be called something else for other DBMS systems http://docs.oracle.com/cd/B19306_01/server.102/b14237/statviews_1069.htm#i1578369 ). I guess it also boils down to what you expect a provider to give you from GetIndexes (currently unclear). If that was documented, then it would probably make sense for a provider to limit the list of "indexes" returned to be actually something useful for reverse engineering

@lajones
Copy link
Contributor

lajones commented Dec 4, 2015

@ErikEJ Yes - this is for SQL Server only i.e. SqlServerDatabaseModelFactory.GetIndexes() would add an appropriate IndexModel to the DatabaseModel if type_desc is good and would log the warning saying why not if not. Other providers would be free to do different things / issue their own warning messages.

Current plans for the IndexModel objects are to support the functionality listed in #3710. Though I agree that when this is finalized it would be good to document what we expect all the *Model objects to contain.

@ErikEJ ErikEJ closed this Dec 4, 2015
@ErikEJ ErikEJ reopened this Dec 5, 2015
@ErikEJ
Copy link
Contributor Author

ErikEJ commented Dec 5, 2015

So you prefer to deal with potentially hard to understand warnings for unsupported index types?

@ErikEJ ErikEJ closed this Dec 5, 2015
@lajones
Copy link
Contributor

lajones commented Dec 7, 2015

Well I hope the warnings will be as helpful as we can make them, but, yes, essentially I'd rather have warnings rather than just ignore them without explanation.

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.

4 participants