Skip to content

Conversation

alinaliBQ
Copy link
Contributor

@alinaliBQ alinaliBQ commented Oct 6, 2025

Rationale for this change

Add placeholders for ODBC APIs to make it easier to create draft PRs for ODBC APIs

What changes are included in this PR?

Add placeholders for ODBC APIs and link the corresponding GitHub issues.
Minor build fixes

Are these changes tested?

Tested on local machine

Are there any user-facing changes?

No

Copy link

github-actions bot commented Oct 6, 2025

⚠️ GitHub issue #47697 has been automatically assigned in GitHub to PR creator.

Update entry_points.cc

Update odbc_api.cc
@alinaliBQ alinaliBQ force-pushed the gh-47697-add-api-placeholders branch from 63a95cd to 833824a Compare October 6, 2025 22:38
@alinaliBQ alinaliBQ marked this pull request as ready for review October 6, 2025 23:39
@alinaliBQ alinaliBQ requested a review from lidavidm as a code owner October 6, 2025 23:39
@alinaliBQ
Copy link
Contributor Author

@kou @lidavidm Please review this PR, it is extracted from #46099. After this PR is merged, my team can start creating the draft PRs for ODBC API

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Oct 7, 2025
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

SQLSMALLINT fk_catalog_name_length, SQLWCHAR* fk_schema_name,
SQLSMALLINT fk_schema_name_length, SQLWCHAR* fk_table_name,
SQLSMALLINT fk_table_name_length) {
ARROW_LOG(DEBUG) << "SQLForeignKeysW called with stmt: " << stmt
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ARROW_LOG(DEBUG) << "SQLForeignKeysW called with stmt: " << stmt
ARROW_LOG(DEBUG) << "SQLForeignKeys called with stmt: " << stmt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unicode support will be added in upcoming PRs. Some ODBC unicode APIs will have a W at the end after compilation, because some of the function names are macros that expand to names with W. Please see:
https://github.com/microsoft/ODBC-Specification/blob/4dda95986bda5d3b55d7749315d3e5a0951c1e50/Windows/inc/sqlucode.h#L952-L996

Copy link
Member

Choose a reason for hiding this comment

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

OK. This may be better then:

Suggested change
ARROW_LOG(DEBUG) << "SQLForeignKeysW called with stmt: " << stmt
ARROW_LOG(DEBUG) << #SQLColAttribute " called with stmt: " << stmt

<< ", fk_table_name: " << static_cast<const void*>(fk_table_name)
<< ", fk_table_name_length: " << fk_table_name_length;
return ODBC::ODBCStatement::ExecuteWithDiagnostics(stmt, SQL_ERROR, [=]() {
throw driver::odbcabstraction::DriverException("SQLForeignKeysW is not implemented",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw driver::odbcabstraction::DriverException("SQLForeignKeysW is not implemented",
throw driver::odbcabstraction::DriverException("SQLForeignKeys is not implemented",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

Comment on lines +242 to +243
<< ", pk_catalog_name: " << static_cast<const void*>(pk_catalog_name)
<< ", pk_catalog_name_length: " << pk_catalog_name_length
Copy link
Member

Choose a reason for hiding this comment

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

Should we print as string...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get the variable names in string, the user can use ODBC driver manager to trace the API calls and the inputs. I hope printing the addresses for now is ok

SQLSMALLINT catalog_name_length, SQLWCHAR* schema_name,
SQLSMALLINT schema_name_length, SQLWCHAR* table_name,
SQLSMALLINT table_name_length) {
ARROW_LOG(DEBUG) << "SQLPrimaryKeysW called with stmt: " << stmt
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ARROW_LOG(DEBUG) << "SQLPrimaryKeysW called with stmt: " << stmt
ARROW_LOG(DEBUG) << "SQLPrimaryKeys called with stmt: " << stmt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

<< ", table_name: " << static_cast<const void*>(table_name)
<< ", table_name_length: " << table_name_length;
return ODBC::ODBCStatement::ExecuteWithDiagnostics(stmt, SQL_ERROR, [=]() {
throw driver::odbcabstraction::DriverException("SQLPrimaryKeysW is not implemented",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw driver::odbcabstraction::DriverException("SQLPrimaryKeysW is not implemented",
throw driver::odbcabstraction::DriverException("SQLPrimaryKeys is not implemented",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

<< ", result: " << static_cast<const void*>(result);
// GH-46096 TODO: Implement SQLAllocEnv
// GH-46097 TODO: Implement SQLAllocConnect, pre-requisite requires SQLAllocEnv
// implementation GH-47706 TODO: Implement SQLAllocStmt, pre-requisite requires
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// implementation GH-47706 TODO: Implement SQLAllocStmt, pre-requisite requires
// implementation
// GH-47706 TODO: Implement SQLAllocStmt, pre-requisite requires

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed

// GH-46096 TODO: Implement SQLAllocEnv
// GH-46097 TODO: Implement SQLAllocConnect, pre-requisite requires SQLAllocEnv
// implementation GH-47706 TODO: Implement SQLAllocStmt, pre-requisite requires
// SQLDriverConnect implementation GH-47707 TODO: Implement SQL_HANDLE_DESC for
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// SQLDriverConnect implementation GH-47707 TODO: Implement SQL_HANDLE_DESC for
// SQLDriverConnect implementation
// GH-47707 TODO: Implement SQL_HANDLE_DESC for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed

SQLPOINTER diag_info_ptr, SQLSMALLINT buffer_length,
SQLSMALLINT* string_length_ptr) {
// GH-46573 TODO: Implement additional fields types
ARROW_LOG(DEBUG) << "SQLGetDiagFieldW called with handle_type: " << handle_type
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ARROW_LOG(DEBUG) << "SQLGetDiagFieldW called with handle_type: " << handle_type
ARROW_LOG(DEBUG) << "SQLGetDiagField called with handle_type: " << handle_type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

SQLWCHAR* sql_state, SQLINTEGER* native_error_ptr,
SQLWCHAR* message_text, SQLSMALLINT buffer_length,
SQLSMALLINT* text_length_ptr) {
ARROW_LOG(DEBUG) << "SQLGetDiagRecW called with handle_type: " << handle_type
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ARROW_LOG(DEBUG) << "SQLGetDiagRecW called with handle_type: " << handle_type
ARROW_LOG(DEBUG) << "SQLGetDiagRec called with handle_type: " << handle_type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

Copy link
Contributor Author

@alinaliBQ alinaliBQ left a comment

Choose a reason for hiding this comment

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

Addressing @kou's comments

SQLPOINTER diag_info_ptr, SQLSMALLINT buffer_length,
SQLSMALLINT* string_length_ptr) {
// GH-46573 TODO: Implement additional fields types
ARROW_LOG(DEBUG) << "SQLGetDiagFieldW called with handle_type: " << handle_type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

SQLWCHAR* sql_state, SQLINTEGER* native_error_ptr,
SQLWCHAR* message_text, SQLSMALLINT buffer_length,
SQLSMALLINT* text_length_ptr) {
ARROW_LOG(DEBUG) << "SQLGetDiagRecW called with handle_type: " << handle_type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

SQLSMALLINT catalog_name_length, SQLWCHAR* schema_name,
SQLSMALLINT schema_name_length, SQLWCHAR* table_name,
SQLSMALLINT table_name_length) {
ARROW_LOG(DEBUG) << "SQLPrimaryKeysW called with stmt: " << stmt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

<< ", table_name: " << static_cast<const void*>(table_name)
<< ", table_name_length: " << table_name_length;
return ODBC::ODBCStatement::ExecuteWithDiagnostics(stmt, SQL_ERROR, [=]() {
throw driver::odbcabstraction::DriverException("SQLPrimaryKeysW is not implemented",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

<< ", fk_table_name: " << static_cast<const void*>(fk_table_name)
<< ", fk_table_name_length: " << fk_table_name_length;
return ODBC::ODBCStatement::ExecuteWithDiagnostics(stmt, SQL_ERROR, [=]() {
throw driver::odbcabstraction::DriverException("SQLForeignKeysW is not implemented",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #47725 (comment)

Comment on lines +242 to +243
<< ", pk_catalog_name: " << static_cast<const void*>(pk_catalog_name)
<< ", pk_catalog_name_length: " << pk_catalog_name_length
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get the variable names in string, the user can use ODBC driver manager to trace the API calls and the inputs. I hope printing the addresses for now is ok

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 8, 2025
@kou kou merged commit 353a0a2 into apache:main Oct 8, 2025
43 checks passed
@kou kou removed the awaiting changes Awaiting changes label Oct 8, 2025
Copy link

After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit 353a0a2.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

@alinaliBQ
Copy link
Contributor Author

Thanks for merging it

zanmato1984 pushed a commit to zanmato1984/arrow that referenced this pull request Oct 15, 2025
)

### Rationale for this change

Add placeholders for ODBC APIs to make it easier to create draft PRs for ODBC APIs

### What changes are included in this PR?

Add placeholders for ODBC APIs and link the corresponding GitHub issues. 
Minor build fixes
### Are these changes tested?

Tested on local machine

### Are there any user-facing changes?
No
* GitHub Issue: apache#47697

Authored-by: Alina (Xi) Li <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants