ODBC: Fix for data loading failure in Power BI Desktop #627
Conversation
- add docs
- add unit test - add errors - enable direct query
…e/odbc/pbi # Conflicts: # sql-odbc/.gitignore # sql-odbc/src/PowerBIConnector/OdfeSqlOdbcPBIConnector.mproj # sql-odbc/src/PowerBIConnector/OdfeSqlOdbcPBIConnector.pq # sql-odbc/src/PowerBIConnector/OdfeSqlOdbcPBIConnector.query.pq # sql-odbc/src/PowerBIConnector/bin/Release/OdfeSqlOdbcPBIConnector.mez
| @@ -936,7 +937,9 @@ TEST_F(TestSQLDescribeCol, SingleColumnMetadata) { | |||
| EXPECT_EQ(single_col, m_column_name); | |||
| EXPECT_EQ(single_col_name_length, m_column_name_length); | |||
| EXPECT_EQ(single_col_data_type, m_data_type); | |||
| EXPECT_EQ(single_col_column_size, m_column_size); | |||
| // Value changes according to pagination setup on server | |||
There was a problem hiding this comment.
Not sure why. Created an issue #628 to investigate this further since I got one another value as 23 for server 1.9.0.
Will revert changes in this PR and handle this separately as it's not a blocker for fixing the data loading issue in Power BI.
|
|
||
| for (size_t i = 0; i < binds.size(); i++) { | ||
| // Add tuples for SQLColumns | ||
| if (binds.size() > COLUMNS_SQL_DATA_TYPE) { |
There was a problem hiding this comment.
Should we just report the extra columns regardless?
There was a problem hiding this comment.
The same method is used while adding tuples for SQLTables as well. SQLTables has only 5 columns whereas SQLColumns has 18.
So this check ensures that the data type column is updated for SQLColumns only. If we remove this check then we get Memory Corruption since SQLTables will try to access column which doesn't exist.
There was a problem hiding this comment.
Almost wondering if it would be worth it to split some of this logic up, now that we're handling special cases for SQLTables & SQLColumns
There was a problem hiding this comment.
Created issue #630 to revisit the logic of assigning data for SQLTables & SQLColumns
| @@ -85,6 +85,9 @@ const std::vector< std::string > flights_data_type = { | |||
| "float", "keyword", "integer", "keyword", "keyword", "keyword", "date", | |||
| "keyword", "keyword", "boolean", "float", "keyword", "keyword", "keyword", | |||
| "keyword", "keyword", "keyword", "keyword"}; | |||
| const std::vector< short > flights_sql_data_type = { | |||
There was a problem hiding this comment.
Can we use SQL type contstants here instead? (eg. SQLWVARCHAR)
| @@ -29,6 +29,7 @@ | |||
| #pragma clang diagnostic pop | |||
| #endif // __APPLE__ | |||
| #include "statement.h" | |||
| #include "es_types.h" | |||
There was a problem hiding this comment.
Already included on L22 (might need to expand GH code preview)
|
|
||
| for (size_t i = 0; i < binds.size(); i++) { | ||
| // Add tuples for SQLColumns | ||
| if (binds.size() > COLUMNS_SQL_DATA_TYPE) { |
There was a problem hiding this comment.
Almost wondering if it would be worth it to split some of this logic up, now that we're handling special cases for SQLTables & SQLColumns
Issue #595
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.