Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

ODBC: Fix for data loading failure in Power BI Desktop #627

Merged
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions sql-odbc/src/IntegrationTests/ITODBCCatalog/test_odbc_catalog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use SQL type contstants here instead? (eg. SQLWVARCHAR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

-9, -9, -7, 7, 7, -9, 4, 7, -9, 4, -9, -9, -9,
1296, -9, -9, -7, 7, -9, -9, -9, -9, -9, -9, -9};
const std::string flights_catalog_odfe = "odfe-cluster";
const std::string flights_catalog_elas = "elasticsearch";
const std::string flights_table_name = "kibana_sample_data_flights";
Expand Down Expand Up @@ -355,15 +358,16 @@ TEST_F(TestSQLColumns, FlightsValidation) {
binds.push_back(bind_info(2, SQL_C_CHAR));
binds.push_back(bind_info(3, SQL_C_CHAR));
binds.push_back(bind_info(4, SQL_C_CHAR));
binds.push_back(bind_info(5, SQL_C_SSHORT));
binds.push_back(bind_info(5, SQL_C_SHORT));
binds.push_back(bind_info(6, SQL_C_CHAR));
binds.push_back(bind_info(7, SQL_C_SLONG));
binds.push_back(bind_info(8, SQL_C_SLONG));
binds.push_back(bind_info(9, SQL_C_SSHORT));
binds.push_back(bind_info(10, SQL_C_SSHORT));
binds.push_back(bind_info(11, SQL_C_SSHORT));
binds.push_back(bind_info(12, SQL_C_CHAR));
binds.push_back(bind_info(13, SQL_C_CHAR));
binds.push_back(bind_info(14, SQL_C_SSHORT));
binds.push_back(bind_info(14, SQL_C_SHORT));
binds.push_back(bind_info(15, SQL_C_SSHORT));
binds.push_back(bind_info(16, SQL_C_SLONG));
binds.push_back(bind_info(17, SQL_C_SLONG));
Expand All @@ -390,16 +394,26 @@ TEST_F(TestSQLColumns, FlightsValidation) {
case 4:
EXPECT_EQ(it.AsString(), flights_column_name[column_idx]);
break;
case 5:
EXPECT_EQ(
it.AsString(),
std::to_string(flights_sql_data_type[column_idx]));
break;
case 6:
EXPECT_EQ(it.AsString(), flights_data_type[column_idx]);
break;
case 9:
case 10:
EXPECT_EQ(it.AsString(), flights_decimal_digits);
break;
case 10:
case 11:
EXPECT_EQ(it.AsString(), flights_num_prec_radix);
break;
case 16:
case 14:
EXPECT_EQ(
it.AsString(),
std::to_string(flights_sql_data_type[column_idx]));
break;
case 17:
EXPECT_EQ(it.AsString(), std::to_string(column_idx + 1));
break;
default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ const int delay_offset_3_1 = 0;
const int delay_offset_3_2 = 180;
const SQLSMALLINT single_col_name_length = 6;
const SQLSMALLINT single_col_data_type = SQL_WVARCHAR;
const SQLULEN single_col_column_size = 25;
const SQLULEN single_col_column_size_25 = 25;
const SQLULEN single_col_column_size_15 = 15;
const SQLSMALLINT single_col_decimal_digit = 0;
const SQLSMALLINT single_col_nullable = 2;
const std::wstring single_row = L"1";
Expand Down Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

EXPECT_TRUE((single_col_column_size_25 == m_column_size)
|| (single_col_column_size_15 == m_column_size));
EXPECT_EQ(single_col_decimal_digit, m_decimal_digits);
EXPECT_EQ(single_col_nullable, m_nullable);
}
Expand Down
40 changes: 38 additions & 2 deletions sql-odbc/src/odfesqlodbc/es_info.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,22 @@ const std::unordered_map< int, std::vector< int > > sql_es_type_map = {
{ES_TYPE_KEYWORD, ES_TYPE_TEXT, ES_TYPE_NESTED, ES_TYPE_OBJECT}},
{SQL_TYPE_TIMESTAMP, {ES_TYPE_DATETIME}}};

const std::unordered_map< std::string, int > data_name_data_type_map = {
{ES_TYPE_NAME_BOOLEAN, SQL_BIT},
{ES_TYPE_NAME_BYTE, SQL_TINYINT},
{ES_TYPE_NAME_SHORT, SQL_SMALLINT},
{ES_TYPE_NAME_INTEGER, SQL_INTEGER},
{ES_TYPE_NAME_LONG, SQL_BIGINT},
{ES_TYPE_NAME_HALF_FLOAT, SQL_REAL},
{ES_TYPE_NAME_FLOAT, SQL_REAL},
{ES_TYPE_NAME_DOUBLE, SQL_DOUBLE},
{ES_TYPE_NAME_SCALED_FLOAT, SQL_DOUBLE},
{ES_TYPE_NAME_KEYWORD, SQL_WVARCHAR},
{ES_TYPE_NAME_TEXT, SQL_WVARCHAR},
{ES_TYPE_NAME_DATE, ES_TYPE_TIMESTAMP},
{ES_TYPE_NAME_OBJECT, SQL_WVARCHAR},
{ES_TYPE_NAME_NESTED, SQL_WVARCHAR}};

// Boilerplate code for easy column bind handling
class BindTemplate {
public:
Expand Down Expand Up @@ -476,8 +492,28 @@ void SetTableTuples(QResultClass *res, const TableResultSet res_type,
// information for its Data Preview window.
std::string catalog("");
bind_tbl[TABLES_CATALOG_NAME]->UpdateData((void *)catalog.c_str(), 0);
for (size_t i = 0; i < binds.size(); i++)
binds[i]->AssignData(&tuple[i]);

for (size_t i = 0; i < binds.size(); i++) {
// Add tuples for SQLColumns
if (binds.size() > COLUMNS_SQL_DATA_TYPE) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just report the extra columns regardless?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created issue #630 to revisit the logic of assigning data for SQLTables & SQLColumns

// Add data type for data loading issue in Power BI Desktop
auto data_type = data_name_data_type_map
.find(bind_tbl[COLUMNS_TYPE_NAME]->AsString())->second;
if (i == COLUMNS_DATA_TYPE) {
set_tuplefield_int2(&tuple[COLUMNS_DATA_TYPE],
static_cast< short >(data_type));
} else if (i == COLUMNS_SQL_DATA_TYPE) {
set_tuplefield_int2(&tuple[COLUMNS_SQL_DATA_TYPE],
static_cast< short >(data_type));
} else {
binds[i]->AssignData(&tuple[i]);
}
}
// Add tuples for SQLTables
else {
binds[i]->AssignData(&tuple[i]);
}
}
};

// General case
Expand Down
31 changes: 16 additions & 15 deletions sql-odbc/src/odfesqlodbc/es_parse_result.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#pragma clang diagnostic pop
#endif // __APPLE__
#include "statement.h"
#include "es_types.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already included on L22 (might need to expand GH code preview)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed


typedef std::vector< std::pair< std::string, OID > > schema_type;
typedef rabbit::array json_arr;
Expand Down Expand Up @@ -69,21 +70,21 @@ static const std::string JSON_KW_CURSOR = "cursor";

// clang-format on
const std::unordered_map< std::string, OID > type_to_oid_map = {
{"boolean", ES_TYPE_BOOL},
{"byte", ES_TYPE_INT2},
{"short", ES_TYPE_INT2},
{"integer", ES_TYPE_INT4},
{"long", ES_TYPE_INT8},
{"half_float", ES_TYPE_FLOAT4},
{"float", ES_TYPE_FLOAT4},
{"double", ES_TYPE_FLOAT8},
{"scaled_float", ES_TYPE_FLOAT8},
{"keyword", ES_TYPE_VARCHAR},
{"text", ES_TYPE_VARCHAR},
{"date", ES_TYPE_TIMESTAMP},
{"object", ES_TYPE_VARCHAR},
{"nested", ES_TYPE_VARCHAR},
{"date", ES_TYPE_DATE}};
{ES_TYPE_NAME_BOOLEAN, ES_TYPE_BOOL},
{ES_TYPE_NAME_BYTE, ES_TYPE_INT2},
{ES_TYPE_NAME_SHORT, ES_TYPE_INT2},
{ES_TYPE_NAME_INTEGER, ES_TYPE_INT4},
{ES_TYPE_NAME_LONG, ES_TYPE_INT8},
{ES_TYPE_NAME_HALF_FLOAT, ES_TYPE_FLOAT4},
{ES_TYPE_NAME_FLOAT, ES_TYPE_FLOAT4},
{ES_TYPE_NAME_DOUBLE, ES_TYPE_FLOAT8},
{ES_TYPE_NAME_SCALED_FLOAT, ES_TYPE_FLOAT8},
{ES_TYPE_NAME_KEYWORD, ES_TYPE_VARCHAR},
{ES_TYPE_NAME_TEXT, ES_TYPE_VARCHAR},
{ES_TYPE_NAME_DATE, ES_TYPE_TIMESTAMP},
{ES_TYPE_NAME_OBJECT, ES_TYPE_VARCHAR},
{ES_TYPE_NAME_VARCHAR, ES_TYPE_VARCHAR},
{ES_TYPE_NAME_DATE, ES_TYPE_DATE}};

#define ES_VARCHAR_SIZE (-2)
const std::unordered_map< OID, int16_t > oid_to_size_map = {
Expand Down
32 changes: 16 additions & 16 deletions sql-odbc/src/odfesqlodbc/es_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,22 @@ extern "C" {
#define ES_TYPE_LO ? ? ? ? /* waiting for permanent type */
#endif

#define ES_TYPE_NAME_BOOLEAN "boolean";
#define ES_TYPE_NAME_BYTE "byte";
#define ES_TYPE_NAME_SHORT "short";
#define ES_TYPE_NAME_INTEGER "integer";
#define ES_TYPE_NAME_LONG "long";
#define ES_TYPE_NAME_HALF_FLOAT "half_float";
#define ES_TYPE_NAME_FLOAT "float";
#define ES_TYPE_NAME_DOUBLE "double";
#define ES_TYPE_NAME_SCALED_FLOAT "scaled_float";
#define ES_TYPE_NAME_KEYWORD "keyword";
#define ES_TYPE_NAME_TEXT "text";
#define ES_TYPE_NAME_NESTED "nested";
#define ES_TYPE_NAME_DATE "date";
#define ES_TYPE_NAME_OBJECT "object";
#define ES_TYPE_NAME_VARCHAR "varchar";
#define ES_TYPE_NAME_UNSUPPORTED "unsupported";
#define ES_TYPE_NAME_BOOLEAN "boolean"
#define ES_TYPE_NAME_BYTE "byte"
#define ES_TYPE_NAME_SHORT "short"
#define ES_TYPE_NAME_INTEGER "integer"
#define ES_TYPE_NAME_LONG "long"
#define ES_TYPE_NAME_HALF_FLOAT "half_float"
#define ES_TYPE_NAME_FLOAT "float"
#define ES_TYPE_NAME_DOUBLE "double"
#define ES_TYPE_NAME_SCALED_FLOAT "scaled_float"
#define ES_TYPE_NAME_KEYWORD "keyword"
#define ES_TYPE_NAME_TEXT "text"
#define ES_TYPE_NAME_NESTED "nested"
#define ES_TYPE_NAME_DATE "date"
#define ES_TYPE_NAME_OBJECT "object"
#define ES_TYPE_NAME_VARCHAR "varchar"
#define ES_TYPE_NAME_UNSUPPORTED "unsupported"

#define MS_ACCESS_SERIAL "int identity"
#define ES_TYPE_BOOL 16
Expand Down