Updated getIndexInfo() to include Columnstore indexes by using custom query. #2566
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2566 +/- ##
============================================
+ Coverage 51.03% 51.06% +0.02%
- Complexity 3919 3921 +2
============================================
Files 147 147
Lines 33456 33445 -11
Branches 5604 5602 -2
============================================
+ Hits 17074 17078 +4
+ Misses 13971 13956 -15
Partials 2411 2411 ☔ View full report in Codecov by Sentry. |
Ananya2
requested review from
lilgreenbird,
divang,
Jeffery-Wasty,
tkyc and
machavan
| arguments[5] = "E"; | ||
| return getResultSetWithProvidedColumnNames(cat, CallableHandles.SP_STATISTICS, arguments, | ||
| getIndexInfoColumnNames); | ||
| String query = "SELECT " + |
There was a problem hiding this comment.
Can we use StringBuilder instead of String for concatenation?
| @Override | ||
| public java.sql.ResultSet getIndexInfo(String cat, String schema, String table, boolean unique, | ||
| boolean approximate) throws SQLServerException, SQLTimeoutException { | ||
| boolean approximate) throws SQLServerException, SQLTimeoutException, SQLException { |
There was a problem hiding this comment.
Adding a comments to explain the rationale behind replacing the stored procedure call with a direct SQL query.
| @Override | ||
| public java.sql.ResultSet getIndexInfo(String cat, String schema, String table, boolean unique, | ||
| boolean approximate) throws SQLServerException, SQLTimeoutException { | ||
| boolean approximate) throws SQLServerException, SQLTimeoutException, SQLException { |
There was a problem hiding this comment.
Remove SQLServerException and SQLTimeoutException (more general SQLException covers both).
| boolean approximate) throws SQLServerException, SQLTimeoutException, SQLException { | |
| boolean approximate) throws SQLException { |
| private static String col3Name = AbstractSQLGenerator.escapeIdentifier( "p3"); | ||
|
|
||
| @BeforeAll | ||
| public static void setupTests() throws Exception { |
There was a problem hiding this comment.
Try to use the most specific exception possible - replace Exception on lines 29 and 34 with SQLException.
| @@ -0,0 +1,174 @@ | |||
| package com.microsoft.sqlserver.jdbc.databasemetadata; | |||
|
|
|||
| import static org.junit.jupiter.api.Assertions.*; | |||
There was a problem hiding this comment.
Use specific imports.
| import static org.junit.jupiter.api.Assertions.*; | |
| import static org.junit.jupiter.api.Assertions.assertEquals; | |
| import static org.junit.jupiter.api.Assertions.assertNull; | |
| import static org.junit.jupiter.api.Assertions.assertTrue; | |
| import static org.junit.jupiter.api.Assertions.fail; |
|
|
||
| public class DatabaseMetadataGetIndexInfoTest extends AbstractTest { | ||
|
|
||
| private static String tableName = AbstractSQLGenerator.escapeIdentifier("DBMetadataTestTable"); |
There was a problem hiding this comment.
Run through formatter.
|
|
||
| stmt.executeUpdate(createTableSQL); | ||
| assertNull(connection.getWarnings(), "Expecting NO SQLWarnings from 'create table', at Connection."); | ||
| assertNull(stmt.getWarnings(), "Expecting NO SQLWarnings from 'create table', at Statement."); |
There was a problem hiding this comment.
Because these messages are used several times, can each of the Connection and Statement messages be placed in TestResource instead?
| } | ||
|
|
||
| @Test | ||
| public void testGetIndexInfo() throws SQLException, SQLServerException { |
There was a problem hiding this comment.
Exception not needed.
| public void testGetIndexInfo() throws SQLException, SQLServerException { | |
| public void testGetIndexInfo() throws SQLException { |
| String indexType = rs1.getString("IndexType"); | ||
| String indexName = rs1.getString("IndexName"); | ||
|
|
||
| assertEquals(rs1.getString("CatalogName"), rs2.getString("CatalogName")); |
There was a problem hiding this comment.
Can we add all these strings to Constants?
| } | ||
|
|
||
| @Test | ||
| public void testGetIndexInfoCaseSensitivity() throws SQLException, SQLServerException { |
There was a problem hiding this comment.
Exception not needed.
| public void testGetIndexInfoCaseSensitivity() throws SQLException, SQLServerException { | |
| public void testGenIndexInfoCaseSensitivity() throws SQLException { |
| @Test | ||
| public void testGetIndexInfoCaseSensitivity() throws SQLException, SQLServerException { | ||
| ResultSet rs1, rs2 = null; | ||
| try (Connection connection = getConnection(); Statement stmt = connection.createStatement()) { |
There was a problem hiding this comment.
Never used.
| try (Connection connection = getConnection(); Statement stmt = connection.createStatement()) { | |
| try (Connection connection = getConnection()) { |
Replaced the use of the sp_statistics stored procedure with a custom query to retrieve index information as the sp_statistics procedure did not return Columnstore indexes, so a query using sys.indexes was implemented as a workaround.
This new query ensures that all index types (Clustered, NonClustered, Columnstore) are included in the result set.
Github Issue: #2546