-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-48707: [C++][FlightRPC] Use IRD precision/scale defaults with ARD override in SQLGetData #48708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
lidavidm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @alinaliBQ
alinaliBQ
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are failing for me locally because currently SQLGetDescField and SQLSetDescField are not implemented yet, and I have raised #48730. For context, ODBC layer tests are not run in the CI due to blockers.
Maybe we can disable these 2 new tests for now and add it as part of GH-48730 to enable these tests?
|
@alinaliBQ thank you for catching this. I did run this locally .. but seems like something went wrong when I ran this. I addressed those review comments. Thank you! |
da6a2e8 to
7c633fc
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No worries, thanks for addressing the comments! |
Rationale for this change
arrow/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc
Lines 746 to 748 in 8fc54a3
This was introduced by ed36107
The
GetDatafunction was using hardcoded precision (38) and scale (0) values instead of retrieving them from the IRD (Implementation Row Descriptor). This fix ensures that precision/scale are properly retrieved from the IRD as defaults, and can be overridden by ARD (Application Row Descriptor) values when specified, following ODBC specification behavior.What changes are included in this PR?
ODBCStatement::GetData()to use IRD precision/scale as defaults instead of hardcoded valuesSQL_ARD_TYPEorSQL_C_DEFAULTis usedAre these changes tested?
Yes. Added two tests in
statement_test.cc:TestGetDataPrecisionScaleUsesIRDAsDefault: Verifies IRD precision/scale are used as defaultsTestGetDataPrecisionScaleUsesARDWhenSet: Verifies ARD precision/scale override IRD when setAre there any user-facing changes?
I think it's fair to say a no. This is an internal fix that ensures correct ODBC behavior.