Quick fix for CH ODBC with MS Access #275 #516
Conversation
|
|
|
Hi @YasuhiroHarada! We appreciate your contribution! |
…r unimplemented API call
|
Regarding the test failures in GitHub Actions, I tried to address them with the following changes. Could you please take a look and let me know your thoughts? Updates to Performance Tests:
|
|
Integration tests have also been added. |
There was a problem hiding this comment.
Thank you very much, @YasuhiroHarada, for your contribution. I'm really glad you had a moment to look into the issue with MS Access.
Overall, I understand the problem and what you're trying to achieve. However, there are still a few parts I'm unsure about, and I’ve added some questions in my comments.
Could you please take a look and help clarify them?
| SQLSMALLINT NameLength2, | ||
| SQLTCHAR * TableName, | ||
| SQLSMALLINT NameLength3, | ||
| SQLUSMALLINT Unique, |
There was a problem hiding this comment.
Should we return nothing if Unique is set to SQL_INDEX_UNIQUE? Data skipping indexes do not guarantee uniqueness.
| query << ")"; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Isn't this a simplified version of the constraint above (lines 911–917)?
if (is_pattern && !is_odbc_v2) {
if (!isMatchAnythingCatalogFnPatternArg(catalog))
query << " AND isNotNull(TABLE_CAT) AND coalesce(TABLE_CAT, '') LIKE '" << escapeForSQL(catalog) << "'";
}
else if (CatalogName) {
query << " AND isNotNull(TABLE_CAT) AND coalesce(TABLE_CAT, '') == '" << escapeForSQL(catalog) << "'";
}
I understand that this filter didn’t work for you. Could you please provide an example where you encountered a problem?
I believe AND database = {catalog} does not always work, because the catalog name can be a pattern (e.g., %).
| ODBC_CALL_ON_STMT_THROW(hstmt, SQLExecDirect(hstmt, ptcharCast(create_index_query.data()), SQL_NTS)); | ||
| ODBC_CALL_ON_STMT_THROW(hstmt, SQLFreeStmt(hstmt, SQL_CLOSE)); | ||
| // Index creation succeeded | ||
| } catch (const std::exception& e) { |
There was a problem hiding this comment.
This shouldn't be the case in the test environment. That would make sense only if we're intentionally skipping the test in certain environments, but it should never happen implicitly.
| } | ||
|
|
||
| // Test SQLStatistics with actual data skipping index | ||
| TEST_F(CatalogFunctionsTest, SQLStatisticsWithDataSkippingIndex) |
There was a problem hiding this comment.
I'm a bit confused by this test - has it already been implemented, or is it still a work in progress?
| auto table_name = reader.getData<std::string>("TABLE_NAME"); | ||
| auto index_name = reader.getData<std::string>("INDEX_NAME"); | ||
|
|
||
| if (table_name.has_value() && table_name.value() == "test_index_table") { |
There was a problem hiding this comment.
I believe this part is unfinished.
| rc = SQLFetch(hstmt); | ||
| EXPECT_EQ(rc, SQL_NO_DATA); | ||
|
|
||
| ODBC_CALL_ON_STMT_THROW(hstmt, SQLFreeStmt(hstmt, SQL_CLOSE)); |
There was a problem hiding this comment.
This is not necessary ClientTestBase sets hstmt for each test.
This applies to the other tests as well.
| ODBC_CALL_ON_STMT_THROW(hstmt, SQLExecDirect(hstmt, ptcharCast(drop_table_query.data()), SQL_NTS)); | ||
| ODBC_CALL_ON_STMT_THROW(hstmt, SQLFreeStmt(hstmt, SQL_CLOSE)); | ||
| } catch (...) { | ||
| // Ignore cleanup errors |
There was a problem hiding this comment.
They should still be logged, but even in the case of cleanup, it's better for the test to fail - because that will indicate the issue early.
Co-authored-by: Andrew Slabko <andrew.slabko@clickhouse.com>
Co-authored-by: Andrew Slabko <andrew.slabko@clickhouse.com>
Co-authored-by: Andrew Slabko <andrew.slabko@clickhouse.com>
Co-authored-by: Andrew Slabko <andrew.slabko@clickhouse.com>
|
@YasuhiroHarada However, I have a few questions that I hope you can answer, since I am not very familiar with this area. Specifically:
If you can help clarify these points, I'll merge the PR and add any missing parts. Thank you @YasuhiroHarada again for your help, I truly appreciate it! |
|
|
||
| auto func = [&](Statement & statement) { | ||
| const auto catalog = (CatalogName ? toUTF8(CatalogName, NameLength1) : statement.getParent().database); | ||
| const auto schema = (SchemaName ? toUTF8(SchemaName, NameLength2) : ""); | ||
| const auto table = (TableName ? toUTF8(TableName, NameLength3) : ""); | ||
|
|
||
| // Build query to retrieve index/statistics information | ||
| // ClickHouse has indices which we can report as statistics | ||
| std::stringstream query; | ||
| query << "SELECT " | ||
| "cast(database, 'Nullable(String)') AS TABLE_CAT, " | ||
| "cast('', 'Nullable(String)') AS TABLE_SCHEM, " | ||
| "cast(table, 'String') AS TABLE_NAME, " | ||
| "cast(" + toSqlQueryValue(SQL_FALSE) + ", 'Int16') AS NON_UNIQUE, " | ||
| "cast(NULL, 'Nullable(String)') AS INDEX_QUALIFIER, " | ||
| "cast(name, 'Nullable(String)') AS INDEX_NAME, " | ||
| "cast(" + toSqlQueryValue(SQL_INDEX_OTHER) + ", 'Int16') AS TYPE, " | ||
| "cast(1, 'Int16') AS ORDINAL_POSITION, " | ||
| "cast(NULL, 'Nullable(String)') AS COLUMN_NAME, " | ||
| "cast(NULL, 'Nullable(String)') AS ASC_OR_DESC, " | ||
| "cast(NULL, 'Nullable(Int32)') AS CARDINALITY, " | ||
| "cast(NULL, 'Nullable(Int32)') AS PAGES, " | ||
| "cast(NULL, 'Nullable(String)') AS FILTER_CONDITION " | ||
| "FROM system.data_skipping_indices " | ||
| "WHERE (1 == 1)"; | ||
|
|
||
| // Add filters based on provided parameters | ||
| if (!catalog.empty() && catalog != "%") { | ||
| query << " AND database = '" << escapeForSQL(catalog) << "'"; | ||
| } | ||
|
|
||
| if (!table.empty() && table != "%") { | ||
| query << " AND table = '" << escapeForSQL(table) << "'"; | ||
| } | ||
|
|
||
| query << " ORDER BY NON_UNIQUE, TYPE, INDEX_NAME, ORDINAL_POSITION"; | ||
| statement.executeQuery(query.str()); | ||
|
|
||
| return SQL_SUCCESS; | ||
| }; | ||
|
|
||
| return CALL_WITH_TYPED_HANDLE(SQL_HANDLE_STMT, StatementHandle, func); |
There was a problem hiding this comment.
This is how I would implement a minimum working version of this function. I still need to understand what we should show in PAGES; for now, I took the size of the table and divided it by 4096. A better version of this function would also return data for the columns used to sort the data, as well as any other indices. However, this minimal version already makes MS Access work.
| auto func = [&](Statement & statement) { | |
| const auto catalog = (CatalogName ? toUTF8(CatalogName, NameLength1) : statement.getParent().database); | |
| const auto schema = (SchemaName ? toUTF8(SchemaName, NameLength2) : ""); | |
| const auto table = (TableName ? toUTF8(TableName, NameLength3) : ""); | |
| // Build query to retrieve index/statistics information | |
| // ClickHouse has indices which we can report as statistics | |
| std::stringstream query; | |
| query << "SELECT " | |
| "cast(database, 'Nullable(String)') AS TABLE_CAT, " | |
| "cast('', 'Nullable(String)') AS TABLE_SCHEM, " | |
| "cast(table, 'String') AS TABLE_NAME, " | |
| "cast(" + toSqlQueryValue(SQL_FALSE) + ", 'Int16') AS NON_UNIQUE, " | |
| "cast(NULL, 'Nullable(String)') AS INDEX_QUALIFIER, " | |
| "cast(name, 'Nullable(String)') AS INDEX_NAME, " | |
| "cast(" + toSqlQueryValue(SQL_INDEX_OTHER) + ", 'Int16') AS TYPE, " | |
| "cast(1, 'Int16') AS ORDINAL_POSITION, " | |
| "cast(NULL, 'Nullable(String)') AS COLUMN_NAME, " | |
| "cast(NULL, 'Nullable(String)') AS ASC_OR_DESC, " | |
| "cast(NULL, 'Nullable(Int32)') AS CARDINALITY, " | |
| "cast(NULL, 'Nullable(Int32)') AS PAGES, " | |
| "cast(NULL, 'Nullable(String)') AS FILTER_CONDITION " | |
| "FROM system.data_skipping_indices " | |
| "WHERE (1 == 1)"; | |
| // Add filters based on provided parameters | |
| if (!catalog.empty() && catalog != "%") { | |
| query << " AND database = '" << escapeForSQL(catalog) << "'"; | |
| } | |
| if (!table.empty() && table != "%") { | |
| query << " AND table = '" << escapeForSQL(table) << "'"; | |
| } | |
| query << " ORDER BY NON_UNIQUE, TYPE, INDEX_NAME, ORDINAL_POSITION"; | |
| statement.executeQuery(query.str()); | |
| return SQL_SUCCESS; | |
| }; | |
| return CALL_WITH_TYPED_HANDLE(SQL_HANDLE_STMT, StatementHandle, func); | |
| auto func = [&](Statement & statement) { | |
| const auto catalog = (CatalogName ? toUTF8(CatalogName, NameLength1) : statement.getParent().database); | |
| const auto schema = (SchemaName ? toUTF8(SchemaName, NameLength2) : ""); | |
| const auto table = (TableName ? toUTF8(TableName, NameLength3) : ""); | |
| // The driver only provides SQL_TABLE_STAT data, at the moment no information about the indices | |
| std::stringstream query; | |
| query << "SELECT " | |
| "cast(database, 'Nullable(String)') AS TABLE_CAT, " | |
| "cast(NULL, 'Nullable(String)') AS TABLE_SCHEM, " | |
| "cast(name, 'String') AS TABLE_NAME, " | |
| "cast(NULL, 'Nullable(Int16)') AS NON_UNIQUE, " | |
| "cast(NULL, 'Nullable(String)') AS INDEX_QUALIFIER, " | |
| "cast(NULL, 'Nullable(String)') AS INDEX_NAME, " | |
| "cast(" + toSqlQueryValue(SQL_TABLE_STAT) + ", 'Int16') AS TYPE, " | |
| "cast(NULL, 'Nullable(Int16)') AS ORDINAL_POSITION, " | |
| "cast(NULL, 'Nullable(String)') AS COLUMN_NAME, " | |
| "cast(NULL, 'Nullable(String)') AS ASC_OR_DESC, " | |
| "cast(total_rows, 'Nullable(Int32)') AS CARDINALITY, " | |
| "cast(ceil(total_bytes / 4096), 'Nullable(Int32)') AS PAGES, " | |
| "cast(NULL, 'Nullable(String)') AS FILTER_CONDITION " | |
| "FROM system.tables " | |
| "WHERE name = " + toSqlQueryValue(table) + " AND database = " + toSqlQueryValue(catalog); | |
| statement.executeQuery(query.str()); | |
| return SQL_SUCCESS; | |
| }; | |
| return CALL_WITH_TYPED_HANDLE(SQL_HANDLE_STMT, StatementHandle, func); | |
|
Hi everyone, I have some coding experience and would like to help. How can I help to get this fix ready to add to the next release? |
4 participants
This pull request enhances the ODBC driver by improving support for specific SQL functions and adding query-building logic for new functionality. The most important changes include enabling previously unsupported SQL APIs, adding query logic for
SQLSpecialColumnsandSQLStatistics, and refining query filtering for catalog and table information.Enhancements to SQL API Support:
SQL_API_SQLSPECIALCOLUMNSandSQL_API_SQLSTATISTICSby uncommenting their definitions inSQLGetFunctions. This allows the driver to declare support for these APIs.Query Logic Additions:
SQLSpecialColumns, including a placeholder query that returns an empty result set with the expected schema. This ensures compatibility with applications expecting this API.SQLStatisticsto retrieve index and statistics information fromsystem.data_skipping_indices. The query includes filters for catalog and table names, ensuring accurate results based on input parameters.Query Filtering Improvements:
SQLTablesto filter results by catalog name when provided, improving the specificity of returned data.