Fix Object Explorer mislabeling Columnstore Indexes as Clustered/Non-Clustered - #2804
Conversation
…red/Non-Clustered IndexCustomeNodeHelper.GetCustomLabel only checked Index.IsClustered, which is also true for clustered columnstore indexes, causing them to be labeled identically to regular clustered indexes. - Extracted label-building logic into a testable BuildIndexLabel method - Added IndexType checks for ClusteredColumnStoreIndex and NonClusteredColumnStoreIndex, falling back to prior behavior for all other index types - Added new localization strings for the two columnstore label variants - Added unit tests covering both new columnstore cases and confirming no regression for standard clustered/non-clustered indexes
There was a problem hiding this comment.
Pull request overview
Fixes Object Explorer index labeling so clustered/nonclustered columnstore indexes are no longer displayed as regular clustered/non-clustered indexes by incorporating IndexType into the label construction, adding the necessary localized strings, and introducing unit tests around a new pure label-building helper.
Changes:
- Refactors index label creation into
IndexCustomeNodeHelper.BuildIndexLabel(...)and addsIndexType-based handling for clustered/nonclustered columnstore indexes. - Adds localized resource entries for “Clustered Columnstore” and “Nonclustered Columnstore”.
- Adds unit tests covering both columnstore variants plus regression cases for regular clustered/non-clustered indexes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs | Adds unit tests for the extracted index label builder and columnstore cases. |
| src/Microsoft.SqlTools.SqlCore/ObjectExplorer/SmoModel/SmoKeyCustomNode.cs | Implements BuildIndexLabel and adds IndexType checks for columnstore index labeling. |
| src/Microsoft.SqlTools.SqlCore/Localization/sr.strings | Adds new resource keys for columnstore index label parts. |
| src/Microsoft.SqlTools.SqlCore/Localization/sr.resx | Adds localized string values for the new columnstore label parts. |
| src/Microsoft.SqlTools.SqlCore/Localization/sr.cs | Updates generated resource accessor keys/properties to include the new strings. |
Suppressed comments (4)
src/Microsoft.SqlTools.SqlCore/Localization/sr.resx:875
- This newly added .resx entry is mis-indented compared to the surrounding resource nodes. Align the / indentation with the rest of the file for consistency.
<data name="NonClusteredColumnStoreIndex_LabelPart" xml:space="preserve">
<value>Nonclustered Columnstore</value>
<comment></comment>
</data>
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:27
- This assertion hard-codes English resource text even though BuildIndexLabel uses localized SR strings. Build the expected value from SR parts so the test stays stable across cultures and resource updates.
Assert.That(label, Is.EqualTo("IX_Test_NCCS (Non-Unique, Nonclustered Columnstore)"));
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:34
- This assertion hard-codes English resource text even though BuildIndexLabel uses localized SR strings. Use SR label parts in the expected string to avoid culture-dependent test failures.
Assert.That(label, Is.EqualTo("IX_Test_Regular (Unique, Clustered)"));
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:41
- This assertion hard-codes English resource text even though BuildIndexLabel uses localized SR strings. Use SR label parts in the expected string to avoid culture-dependent test failures.
Assert.That(label, Is.EqualTo("IX_Test_Regular2 (Non-Unique, Non-Clustered)"));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public static string NonClusteredColumnStoreIndex_LabelPart | ||
| { | ||
| get { return Keys.GetString(Keys.NonClusteredColumnStoreIndex_LabelPart); } | ||
| } |
| public void BuildIndexLabelShouldReturnClusteredColumnstoreLabel() | ||
| { | ||
| string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_CS", isUnique: false, isClustered: true, IndexType.ClusteredColumnStoreIndex); | ||
| Assert.That(label, Is.EqualTo("IX_Test_CS (Non-Unique, Clustered Columnstore)")); |
|
Aasim Khan (@aasimkhan30) PR is up! Thanks for confirming this belonged in sqltoolsservice rather than vscode-mssql — saved me from chasing it in the wrong repo. Would appreciate a review whenever you get a chance. |
@microsoft-github-policy-service agree |
|
Sahil Kulhar (@Cy-nape) please fix the copilot review comments. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new unit tests currently contain a C# compile error (positional argument after named arguments), and there’s a likely missing SMO property prefetch for IndexType that can impact correctness/performance at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:26
- These calls mix named and positional arguments (a positional argument after named ones), which is a C# compile error. Name the final parameter (indexType) or make all arguments positional.
string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_NCCS", isUnique: false, isClustered: false, IndexType.NonClusteredColumnStoreIndex);
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:33
- These calls mix named and positional arguments (a positional argument after named ones), which is a C# compile error. Name the final parameter (indexType) or make all arguments positional.
string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_Regular", isUnique: true, isClustered: true, IndexType.ClusteredIndex);
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:40
- These calls mix named and positional arguments (a positional argument after named ones), which is a C# compile error. Name the final parameter (indexType) or make all arguments positional.
string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_Regular2", isUnique: false, isClustered: false, IndexType.NonClusteredIndex);
src/Microsoft.SqlTools.SqlCore/Localization/sr.resx:875
- The new resx entry is inconsistently indented versus the surrounding entries, which tends to cause noisy diffs when tooling auto-formats the .resx file later.
<data name="NonClusteredColumnStoreIndex_LabelPart" xml:space="preserve">
<value>Nonclustered Columnstore</value>
<comment></comment>
</data>
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The newly added unit tests won’t compile due to mixing named and positional arguments in BuildIndexLabel(...) calls.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/Microsoft.SqlTools.SqlCore/Localization/sr.cs:1506
- There are trailing spaces after this closing brace. With code style enforced in build, it’s best to avoid introducing trailing whitespace in committed files.
src/Microsoft.SqlTools.SqlCore/Localization/sr.resx:875 - The new NonClusteredColumnStoreIndex_LabelPart entry is formatted inconsistently compared to the surrounding resx entries (indentation). Keeping the standard formatting reduces churn and merge conflicts when resources are updated/generated.
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:33
- This call mixes named and positional arguments; positional arguments cannot appear after named arguments, so this test won’t compile. Name the last parameter (indexType) or make all arguments positional.
string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_Regular", isUnique: true, isClustered: true, IndexType.ClusteredIndex);
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:40
- This call mixes named and positional arguments; positional arguments cannot appear after named arguments, so this test won’t compile. Name the last parameter (indexType) or make all arguments positional.
string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_Regular2", isUnique: false, isClustered: false, IndexType.NonClusteredIndex);
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new unit tests contain C# argument syntax that won’t compile (positional args after named args), blocking the build.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:20
- The expected label is hard-coded in English, but BuildIndexLabel uses localized SR values. Building the expected string from the same SR parts will keep the test culture/locale-independent (consistent with other ObjectExplorer tests that assert SR-based labels).
This issue also appears in the following locations of the same file:
- line 27
- line 34
- line 41
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:40
- This call mixes named and positional arguments; C# disallows positional arguments after named ones, so this test won't compile. Make the final argument named (or make all arguments positional).
string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_Regular2", isUnique: false, isClustered: false, IndexType.NonClusteredIndex);
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:34
- The expected label is hard-coded in English, but BuildIndexLabel uses localized SR values. Building the expected string from the same SR parts will keep the test culture/locale-independent (consistent with other ObjectExplorer tests that assert SR-based labels).
Assert.That(label, Is.EqualTo("IX_Test_Regular (Unique, Clustered)"));
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:41
- The expected label is hard-coded in English, but BuildIndexLabel uses localized SR values. Building the expected string from the same SR parts will keep the test culture/locale-independent (consistent with other ObjectExplorer tests that assert SR-based labels).
Assert.That(label, Is.EqualTo("IX_Test_Regular2 (Non-Unique, Non-Clustered)"));
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:27
- The expected label is hard-coded in English, but BuildIndexLabel uses localized SR values. Building the expected string from the same SR parts will keep the test culture/locale-independent (consistent with other ObjectExplorer tests that assert SR-based labels).
Assert.That(label, Is.EqualTo("IX_Test_NCCS (Non-Unique, Nonclustered Columnstore)"));
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
| [Test] | ||
| public void BuildIndexLabelShouldStillReturnClusteredLabelForRegularIndex() | ||
| { | ||
| string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_Regular", isUnique: true, isClustered: true, IndexType.ClusteredIndex); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new unit tests currently won’t compile due to named/positional argument ordering, and the updated .resx includes inconsistent line-ending characters on newly added lines.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:40
- This test call mixes named arguments with a positional argument at the end. In C#, positional arguments cannot follow named arguments, so this will not compile.
string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_Regular2", isUnique: false, isClustered: false, IndexType.NonClusteredIndex);
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
| <value>Nonclustered Columnstore</value> | ||
| <comment></comment> |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The newly added unit test file contains a C# compilation error (named + positional argument mix) that will break the build.
Review details
Suppressed comments (2)
test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/IndexCustomeNodeHelperTests.cs:40
- This call mixes named arguments with a trailing positional argument, which is a C# compile error (positional arguments cannot follow named arguments). Pass the last argument using the
indexType:name as well.
string label = IndexCustomeNodeHelper.BuildIndexLabel("IX_Test_Regular2", isUnique: false, isClustered: false, IndexType.NonClusteredIndex);
src/Microsoft.SqlTools.SqlCore/Localization/sr.cs:1507
- There are trailing spaces after the closing brace on this line, which creates noisy diffs and can violate whitespace/style checks depending on editorconfig settings. Remove the extra whitespace.
public static string NonClusteredColumnStoreIndex_LabelPart
{
get { return Keys.GetString(Keys.NonClusteredColumnStoreIndex_LabelPart); }
}
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
What
Object Explorer was labeling Clustered and Nonclustered Columnstore Indexes identically to regular clustered/non-clustered indexes, since IndexCustomeNodeHelper.GetCustomLabel only checked Index.IsClustered
and never Index.IndexType.
Fixes microsoft/vscode-mssql#22713
Change
Testing
Added IndexCustomeNodeHelperTests.cs — 4 unit tests covering:
All 4 pass locally; full SqlCore build succeeds.