GH-47719: [C++][FlightRPC] Extract SQLTables Implementation#48021
GH-47719: [C++][FlightRPC] Extract SQLTables Implementation#48021lidavidm merged 2 commits intoapache:mainfrom
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
github-actions
bot
added
Component: C++
Component: FlightRPC
awaiting review
| namespace { | ||
| // Helper Functions | ||
|
|
||
| std::wstring GetStringColumnW(SQLHSTMT stmt, int colId) { |
There was a problem hiding this comment.
we might consider moving this GetStringColumnW to a more accessible place shared by all tests
There was a problem hiding this comment.
Moved to odbc_test_suite.
github-actions
bot
added
awaiting committer review
justing-bq
changed the title
|
|
|
@lidavidm Could we get a review? Thanks. |
github-actions
bot
added
awaiting merge
justing-bq
force-pushed
the
sqlTables
branch
from
1354b05 to
83b5b7b
Compare
|
I have rebased and tested the changes locally on MSVC |
|
|
| // Values are nulls | ||
| EXPECT_EQ(SQL_SUCCESS, | ||
| SQLTables(this->stmt, 0, sizeof(catalog_name), 0, sizeof(schema_name), 0, | ||
| sizeof(table_name), 0, sizeof(table_type))); |
There was a problem hiding this comment.
Same here, let's be explicit with nullptr instead of 0
| // All values and sizes are nulls | ||
| EXPECT_EQ(SQL_SUCCESS, SQLTables(this->stmt, 0, 0, 0, 0, 0, 0, 0, 0)); |
There was a problem hiding this comment.
fixed. Btw the sizes are supposed to be zeros instead of nulls, I have fixed the comment also
github-actions
bot
added
awaiting changes
Co-authored-by: justing-bq <justin.gossett@improving.com> Co-authored-by: alinalibq <alina.li@improving.com> Co-Authored-By: rscales <robscales@icloud.com>
Address comment from David
github-actions
bot
added
awaiting change review
alinaliBQ
left a comment
alinaliBQ
left a comment
There was a problem hiding this comment.
Addressed comments from David. I will also check nullptr usages in other open PRs
| // Values are nulls | ||
| EXPECT_EQ(SQL_SUCCESS, | ||
| SQLTables(this->stmt, 0, sizeof(catalog_name), 0, sizeof(schema_name), 0, | ||
| sizeof(table_name), 0, sizeof(table_type))); |
| // All values and sizes are nulls | ||
| EXPECT_EQ(SQL_SUCCESS, SQLTables(this->stmt, 0, 0, 0, 0, 0, 0, 0, 0)); |
There was a problem hiding this comment.
fixed. Btw the sizes are supposed to be zeros instead of nulls, I have fixed the comment also
github-actions
bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit cb31765. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
3 participants
Rationale for this change
Addresses #47719
What changes are included in this PR?
SQLTables enabled. Table tests added.
Are these changes tested?
Tested locally on MSVC.
Are there any user-facing changes?
No.