GH-47721: [C++][FlightRPC] Return ODBC Column Attribute from result set #48050
Conversation
|
|
github-actions
bot
added
Component: C++
Component: FlightRPC
awaiting review
github-actions
bot
added
awaiting committer review
alinaliBQ
force-pushed
the
gh-47721-sql-col-attribute
branch
from
44deffe to
f3e267e
Compare
alinaliBQ
changed the title
|
@lidavidm Draft PR is ready for review 🙂 |
|
@alinaliBQ sorry but do you mind just undrafting PRs when they are ready? That way it pings me automatically and if I approve, I can merge directly |
| return true; | ||
| default: | ||
| return false; | ||
| return true; |
There was a problem hiding this comment.
Won't this give a bad result on non-numeric types? (Maybe the right answer is to throw?)
There was a problem hiding this comment.
In any case, there's is_unsigned_integer in type_traits.h...I don't see why listing out the signed types explicitly here makes any difference if you aren't going to throw for non-numeric types
There was a problem hiding this comment.
Now using arrow::is_unsigned_integer() instead.
We don't want to throw an exception for non-numeric types because we expect all the columns of a result set to hit this logic regardless of type. Returning false for non-numerics is the desired behavior.
| } | ||
|
|
||
| std::string FlightSqlResultSetMetadata::GetTypeName(int column_position) { | ||
| std::string FlightSqlResultSetMetadata::GetTypeName(int column_position, int data_type) { |
There was a problem hiding this comment.
Is there a more precise type we can use than int?
| void CheckSQLColAttributes(SQLHSTMT stmt, SQLUSMALLINT idx, | ||
| const std::wstring& expected_column_name, | ||
| SQLLEN expected_data_type, SQLLEN expected_display_size, | ||
| SQLLEN expected_prec_scale, SQLLEN expected_length, | ||
| SQLLEN expected_column_size, SQLLEN expected_column_scale, | ||
| SQLLEN expected_column_nullability, SQLLEN expected_searchable, | ||
| SQLLEN expected_unsigned_column) { |
There was a problem hiding this comment.
Is the difference just which attributes are checked? Is there any better way to name this? Otherwise I think they should just be overloads of the same function
There was a problem hiding this comment.
No this is testing a different ODBC API. The earlier function was testing SQLColAttribute (singular attribute) while this one is testing SQLColAttributes (plural attributes). Understandably this difference is very easy to miss.
github-actions
bot
added
awaiting changes
Hi @lidavidm I was out of office yesterday, will reply to your comment on #46099 (comment). Please see my reply in #46099 (comment) |
justing-bq
force-pushed
the
gh-47721-sql-col-attribute
branch
from
4395e8a to
d46ee0c
Compare
github-actions
bot
added
awaiting change review
|
@lidavidm Please take another look. |
Co-Authored-By: justing-bq <[email protected]> Co-Authored-By: alinalibq <[email protected]>
justing-bq
force-pushed
the
gh-47721-sql-col-attribute
branch
from
d46ee0c to
e2a3362
Compare
3 participants
Rationale for this change
Implement ODBC to return column attribute values from the result set
What changes are included in this PR?
Are these changes tested?
Tested on local MSVC
Are there any user-facing changes?
N/A