Skip to content

impl(bq_driver): metadata descriptors for SQLPrimaryKeys and SQLForeignKeys#1292

Open
Anshu6250 wants to merge 6 commits into
mainfrom
metadata_descriptor_pri_and_sec
Open

impl(bq_driver): metadata descriptors for SQLPrimaryKeys and SQLForeignKeys#1292
Anshu6250 wants to merge 6 commits into
mainfrom
metadata_descriptor_pri_and_sec

Conversation

@Anshu6250
Copy link
Copy Markdown
Collaborator

@Anshu6250 Anshu6250 commented Nov 14, 2025

This PR adds metadata descriptors for SQLPrimaryKeys and SQLForeignKeys, similar to what we already do for SQLTables and SQLColumns.

@Anshu6250 Anshu6250 changed the title impl(bq_driver): added metadata descriptors for SQLPrimaryKeys and SQ… impl(bq_driver): metadata descriptors for SQLPrimaryKeys and SQLForeignKeys Nov 14, 2025
// ODBCBQClient::GetAllQueryResults() to fetch all the results. In this case,
// the GetQueryResults will be populated in DSResults structure.
//
static std::map<std::string, ColumnSchema> const kODBCForeignKeysMap = {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove ODBC From variable name

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

// ODBCBQClient::GetAllQueryResults() to fetch all the results. In this case,
// the GetQueryResults will be populated in DSResults structure.
//
static std::map<std::string, ColumnSchema> const kODBCPrimaryKeysMap = {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove ODBC from variable name

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

type_info.col_size,
descriptor_record.precision);
auto const& col_name = res.name;
bool is_128_wvarchar_col =
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please follow variable naming convection, remove numerical value from variable name.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

col_name == "PKTABLE_SCHEM" || col_name == "PKTABLE_NAME" ||
col_name == "FKTABLE_SCHEM" || col_name == "FKTABLE_NAME";

if (is_128_wvarchar_col || is_1024_wvarchar_col) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

descriptor_record.local_type_name = "WVARCHAR";
(void)descriptor_record.SetConciseType(SQL_WVARCHAR,
DescriptorType::kIRD);
descriptor_record.length = is_1024_wvarchar_col ? 1024 : 128;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

SQLSMALLINT col_count = 0;
ret = SQLNumResultCols(conn->hstmt, &col_count);
ASSERT_TRUE(SQL_SUCCEEDED(ret));
std::cout << "num cols = " << col_count << std::endl;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

remove this std::cout

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

&data_type, &col_size, &decimal_digits, &nullable);
ASSERT_TRUE(SQL_SUCCEEDED(ret));

std::cout << "Column " << i << ": Name='" << col_name
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

@NeerajDwivedii
Copy link
Copy Markdown
Collaborator

Remove all std::cout which in internal and test case implementation

}

TEST(StatementTest, Check_SQL_Primary_key) {
auto conn = std::make_shared<ODBCHandles>();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these test cases will pass even on main branch as SQLNumResultCols is succedding even on main with column count as 0 and for loop won't be triggered, please modify them to check for explicit column count number

Copy link
Copy Markdown
Collaborator Author

@Anshu6250 Anshu6250 Nov 26, 2025

Choose a reason for hiding this comment

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

test cases are now updated

Comment thread ci/cloudbuild/builds/.typos.toml Outdated
Comment on lines +1 to +2
# Ignores this word globally across the project
[default.extend-words]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Revert this file changes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This change is necessary because the checkers were failing

type_info.col_size,
descriptor_record.precision);
std::string const& col_name = res.name;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

have you verified with simba? , each group taking same descriptor values

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes i have verified

}

TEST(StatementTest, Check_SQL_Primary_key) {
auto conn = std::make_shared<ODBCHandles>();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

see to combine the common functionality in both the test cases in one single function as can see a lot of common functionality including the ExpectedCol structure

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

@Anshu6250 Anshu6250 force-pushed the metadata_descriptor_pri_and_sec branch from 86fe8df to cf8da38 Compare November 26, 2025 08:35
@sachinpro sachinpro marked this pull request as ready for review November 27, 2025 11:24
@sachinpro sachinpro requested a review from a team November 27, 2025 11:24
@Anshu6250 Anshu6250 requested a review from sachinpro December 2, 2025 08:17
@sachinpro sachinpro force-pushed the metadata_descriptor_pri_and_sec branch from cf8da38 to 47d13cf Compare December 9, 2025 15:52
@Anshu6250 Anshu6250 force-pushed the metadata_descriptor_pri_and_sec branch from 47d13cf to 48e7f73 Compare December 17, 2025 05:55
{"TABLE_SCHEM", ColumnSchema{1, BQDataType::kString}},
{"TABLE_NAME", ColumnSchema{2, BQDataType::kString}},
{"COLUMN_NAME", ColumnSchema{3, BQDataType::kString}},
{"KEY_SEQ", ColumnSchema{4, BQDataType::kInt64}},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can probably add KEY_SEQ and PK_NAME to kODBCColumnsMap.

Please see if code duplication can be avoided in other places as well.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Anshu6250 Any update on this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

hi @sachinpro
Yes, I’ve updated this. I introduced a common schema containing shared metadata columns like TABLE_CAT, TABLE_SCHEM, TABLE_NAME, COLUMN_NAME, KEY_SEQ, and PK_NAME, and reused it to avoid duplication.

@Anshu6250 Anshu6250 force-pushed the metadata_descriptor_pri_and_sec branch from 24256da to 6daa6d9 Compare December 22, 2025 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants