impl(bq_driver): SQLStatistics Implementation - #1659
KanchanShu wants to merge 3 commits into
Conversation
d23c914 to
20528df
Compare
KanchanShu
changed the title
| ASSERT_TRUE(status == SQL_SUCCESS || status == SQL_SUCCESS_WITH_INFO); | ||
| ++row_count; | ||
| } | ||
| EXPECT_GE(row_count, 0); |
There was a problem hiding this comment.
Please use exact value to match here and elsewhere
There was a problem hiding this comment.
Done. Added flags according to the drivers
|
|
||
| // Returns a ResultSet containing table-level statistics for the given BigQuery | ||
| // table. BigQuery does not support traditional indexes, so only a | ||
| // SQL_TABLE_STAT row (TYPE = 0) is returned with the row count in CARDINALITY. |
There was a problem hiding this comment.
where are we returning SQL_TABLE_STAT row?
There was a problem hiding this comment.
returning it as part of result set
| // See: | ||
| // https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqlstatistics-function | ||
| StatusRecordOr<ResultSet> FetchStatisticsResultSet( | ||
| StatementHandle& stmt_handle, std::string const& catalog_name, |
There was a problem hiding this comment.
catalog_name, schema_name, table_name and other parameters are not used in the code
|
|
||
| // Resolve catalog (project). Per the spec, catalog_name is not a search | ||
| // pattern. When null/empty, use the connection's current catalog. | ||
| std::string s_catalog_name; |
There was a problem hiding this comment.
catalog_name, s_schema_name , s_table_name seems not to be used anywhere
16fab7c to
f062554
Compare
|
|
||
| // 7: TYPE — SQL_TABLE_STAT (0) indicates this is a table statistics row | ||
| DSValue ds_type; | ||
| ArithmeticToDSValue<SQLSMALLINT>(static_cast<SQLSMALLINT>(SQL_TABLE_STAT), |
There was a problem hiding this comment.
have the type as SQLBIGINT here, this column is declared kTypeSchema{0, BQDataType::kInt64} and we have kInt64 handling as SQLBIGINT everywhere
|
/gcbrun |
| #ifndef BQ_DRIVER_INTEGRATION_TESTS | ||
| EXPECT_EQ(0, row_count); | ||
| #else | ||
| EXPECT_EQ(1, row_count); |
There was a problem hiding this comment.
Why are we deviating from the existing driver here?
There was a problem hiding this comment.
We are deviating here because our implementation strictly follows the official ODBC specification, whereas the existing driver took a shortcut.
According to the Microsoft ODBC specification for SQLStatistics, when a table exists, the driver is supposed to return a baseline row containing statistics for the table itself (TYPE = SQL_TABLE_STAT), followed by rows for any indexes.
Because BigQuery doesn't use traditional RDBMS indexes, the existing driver appears to have simply stubbed out this function to return an empty result set, completely skipping that baseline table-statistics row.
By taking the time to implement it, we are able to fetch BigQuery's table metadata (like numRows) and map it to the CARDINALITY column in the SQL_TABLE_STAT row. This provides ODBC clients with the actual table-level statistics (row count, size) that they expect to receive, making our driver more compliant and feature-complete than the existing one.
There was a problem hiding this comment.
That's fine. We should validate the relevant values then.
Also, have we tested this e2e with MS Access?
| #ifndef BQ_DRIVER_INTEGRATION_TESTS | ||
| SQLSMALLINT const table_name_nullable = SQL_NULLABLE; | ||
| #else | ||
| SQLSMALLINT const table_name_nullable = SQL_NO_NULLS; |
There was a problem hiding this comment.
Why are we deviating from the existing driver here?
There was a problem hiding this comment.
According to the ODBC specification, the TABLE_NAME column in catalog functions cannot be null, so its metadata should report SQL_NO_NULLS. However, the existing driver incorrectly reports TABLE_NAME as SQL_NULLABLE specifically for SQLStatistics.
In our new driver, we fixed this so that it correctly reports SQL_NO_NULLS (which is enforced in ApplyMetadataIrdOverrides inside odbc_desc_attr.cc).
|
@KanchanShu Please fix the CI checks and merge conflicts. |
c14f375 to
7629cee
Compare
|
/gcbrun |
3 participants
No description provided.