Skip to content

Address general test code review comments - #125

Merged
alinaliBQ merged 3 commits into
apache-odbcfrom
address-general-test-comments-oct-21
Oct 28, 2025
Merged

Address general test code review comments#125
alinaliBQ merged 3 commits into
apache-odbcfrom
address-general-test-comments-oct-21

Conversation

@alinaliBQ

@alinaliBQ alinaliBQ commented Oct 21, 2025 •
edited
Loading

Copy link
Copy Markdown
  • Move helper functions to anonymous namespace
  • Use static_cast for SQLWCHAR character arrays
  • Use helper function to remove duplicated allochandle code for environment and connection handles
  • Add [[nodiscard]] for ODBC APIs
  • Remove using List= in test suite definitions

@alinaliBQ alinaliBQ changed the title Address general test comments oct 21 Address general test code review comments Oct 21, 2025
@alinaliBQ
alinaliBQ force-pushed the address-general-test-comments-oct-21 branch from 0a486d3 to 4d32a78 Compare October 22, 2025 22:39
@alinaliBQ
alinaliBQ marked this pull request as ready for review October 22, 2025 22:53
@alinaliBQ alinaliBQ mentioned this pull request Oct 23, 2025
Closed
@alinaliBQ

alinaliBQ commented Oct 27, 2025

Copy link
Copy Markdown
Author

This PR needs to be rebased and it depends on #128

@alinaliBQ
alinaliBQ force-pushed the address-general-test-comments-oct-21 branch 3 times, most recently from adae744 to 778ed61 Compare October 27, 2025 23:02
@alinaliBQ alinaliBQ closed this Oct 27, 2025
@alinaliBQ alinaliBQ reopened this Oct 27, 2025
justing-bq
justing-bq reviewed Oct 28, 2025
View reviewed changes
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc
SQLDriverConnect(this->conn, NULL, &connect_str0[0],
static_cast<SQLSMALLINT>(connect_str0.size()), out_str,
kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT))
<< GetOdbcErrorMessage(SQL_HANDLE_DBC, conn);

@justing-bq justing-bq Oct 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How come we are removing << GetOdbcErrorMessage(SQL_HANDLE_DBC, conn);? This is what prints out the error message when things fail.

@justing-bq justing-bq Oct 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Likewise for all the other instances where this is removed.

@alinaliBQ alinaliBQ Oct 28, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added the checks back. I don't have a strong opinion on this one. Originally I didn't want to flood the terminal with the error messages since they could be the same errors, but GetOdbcErrorMessage will still probably be helpful for debugging, especially when a single test is run.

Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc Outdated
// Allocate a connection using alloc handle
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DBC, env, &conn));

#if defined _WIN32 || defined _WIN64

@justing-bq justing-bq Oct 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

David explicitly asked me to change this from #if defined _WIN32 || defined _WIN64 to #if defined _WIN32 in a previous PR.

@alinaliBQ alinaliBQ Oct 28, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for raising this, fixed

Comment thread cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc
}

void FlightSQLOdbcHandleRemoteTestBase::SetUp() {
ODBCRemoteTestBase::SetUp();

@justing-bq justing-bq Oct 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You should add the following after SetUp:

if (skipping_test_) {
return;
}

@alinaliBQ alinaliBQ Oct 28, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks, fixed

@alinaliBQ
Use RAII helper for allocating and freeing env/conn handles
bf16a90 
Avoids duplicated code

Move test helper functions to anonymous namespace

Add `[[nodiscard]]` for ODBC APIs

Addresses community comment:
https://github.com/apache/arrow/pull/47763/files#r2450014186

Remove `using List=` in test suite definitions

Use static_cast for `SQLWCHAR` in type info test

Use static_cast for `SQLWCHAR` in tables test

* use mutable arrays for places where characters cannot be const

Use static_cast for `SQLWCHAR` in columns test

Update comment

Use static_cast for `SQLWCHAR` in SQLGetInfo test
@alinaliBQ
Address Justin's comments
b6f8be2
@alinaliBQ
alinaliBQ force-pushed the address-general-test-comments-oct-21 branch from 778ed61 to b6f8be2 Compare October 28, 2025 20:37
alinaliBQ
alinaliBQ commented Oct 28, 2025
View reviewed changes

@alinaliBQ alinaliBQ left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed Justin's comments

Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc Outdated
// Allocate a connection using alloc handle
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DBC, env, &conn));

#if defined _WIN32 || defined _WIN64

@alinaliBQ alinaliBQ Oct 28, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for raising this, fixed

Comment thread cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc
}

void FlightSQLOdbcHandleRemoteTestBase::SetUp() {
ODBCRemoteTestBase::SetUp();

@alinaliBQ alinaliBQ Oct 28, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks, fixed

Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc
SQLDriverConnect(this->conn, NULL, &connect_str0[0],
static_cast<SQLSMALLINT>(connect_str0.size()), out_str,
kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT))
<< GetOdbcErrorMessage(SQL_HANDLE_DBC, conn);

@alinaliBQ alinaliBQ Oct 28, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added the checks back. I don't have a strong opinion on this one. Originally I didn't want to flood the terminal with the error messages since they could be the same errors, but GetOdbcErrorMessage will still probably be helpful for debugging, especially when a single test is run.

@alinaliBQ
alinaliBQ merged commit d277098 into apache-odbc Oct 28, 2025
28 of 33 checks passed
@alinaliBQ
alinaliBQ deleted the address-general-test-comments-oct-21 branch October 28, 2025 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justing-bq justing-bq justing-bq approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alinaliBQ @justing-bq