GH-47710: [C++][FlightRPC] Statement attribute Support in ODBC#47773
Conversation
|
|
lidavidm
left a comment
There was a problem hiding this comment.
Same general comments as the other PRs.
42f4f14 to
1e025c7
Compare
|
we worked on:
|
1e025c7 to
be51a05
Compare
|
Rebased on top of |
| namespace { | ||
| // Helper Functions | ||
|
|
||
| // Validate SQLULEN return value | ||
| void ValidateGetStmtAttr(SQLHSTMT statement, SQLINTEGER attribute, | ||
| SQLULEN expected_value) { | ||
| SQLULEN value = 0; | ||
| SQLINTEGER string_length = 0; | ||
|
|
||
| ASSERT_EQ(SQL_SUCCESS, | ||
| SQLGetStmtAttr(statement, attribute, &value, sizeof(value), &string_length)); | ||
|
|
||
| EXPECT_EQ(expected_value, value); | ||
| } |
There was a problem hiding this comment.
Instead of having variants for each type of comparison (equal, greater than, etc.) wouldn't it be better to just have a getter for each type, and keep the assertion in the tests themselves? (It could throw on failure, for instance, to keep the signature simple.)
There was a problem hiding this comment.
Yea that makes sense. I added getters, please let me know what you think
be51a05 to
aebac29
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
worked on comment
| namespace { | ||
| // Helper Functions | ||
|
|
||
| // Validate SQLULEN return value | ||
| void ValidateGetStmtAttr(SQLHSTMT statement, SQLINTEGER attribute, | ||
| SQLULEN expected_value) { | ||
| SQLULEN value = 0; | ||
| SQLINTEGER string_length = 0; | ||
|
|
||
| ASSERT_EQ(SQL_SUCCESS, | ||
| SQLGetStmtAttr(statement, attribute, &value, sizeof(value), &string_length)); | ||
|
|
||
| EXPECT_EQ(expected_value, value); | ||
| } |
There was a problem hiding this comment.
Yea that makes sense. I added getters, please let me know what you think
0675c21 to
bafe934
Compare
Co-Authored-By: rscales <robscales@icloud.com>
Add tests for setting and getting statement attributes Co-Authored-By: rscales <robscales@icloud.com>
- use `platform.h` - move `connect/disconnect` to `setup/teardown` - in-progress on subclassing test fixture Co-authored-by: justing-bq <justin.gossett@improving.com> Co-authored-by: alinalibq <alina.li@improving.com>
- remove `using List = std::list<T>;`
bafe934 to
501eab7
Compare
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 430d4b1. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…pache#47773) ### Rationale for this change Support for getting and setting statement attributes in ODBC is added. ### What changes are included in this PR? - Implementation of `SQLGetStmtAttr` and `SQLSetStmtAttr` to get and set statement attributes. - Tests ### Are these changes tested? Tested on local MSVC ### Are there any user-facing changes? No * GitHub Issue: apache#47710 Lead-authored-by: Alina (Xi) Li <alina.li@improving.com> Co-authored-by: alinalibq <alina.li@improving.com> Co-authored-by: justing-bq <justin.gossett@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Rationale for this change
Support for getting and setting statement attributes in ODBC is added.
What changes are included in this PR?
SQLGetStmtAttrandSQLSetStmtAttrto get and set statement attributes.Are these changes tested?
Tested on local MSVC
Are there any user-facing changes?
No