-
Notifications
You must be signed in to change notification settings - Fork 173
fix(go/adbc): Forward SQLSTATE and vendor code #2801
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
Conversation
| // If there are no details, but we have a `VendorCode`, let's override | ||
| // `vendor_code` and not populate `private_data` with the error details. | ||
| if numDetails == 0 && adbcError.VendorCode != 0 && adbcError.VendorCode != C.ADBC_ERROR_VENDOR_CODE_PRIVATE_DATA { | ||
| err.vendor_code = C.int(adbcError.VendorCode) |
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.
@zeroshade if you are OK with this approach I will change the go.tmpl file and make regenerate.
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.
This looks good to me. I wonder if there's any good way we could add a test for this (perhaps using the sqlite driver or otherwise?)
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 SQLite driver is not written in Go, so I wouldn't be able to exercise this code with it. And the individual Go driver tests seem to skip the FFI layer.
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.
So this is strictly an issue with the C Interop layer? There are interop tests in both the Rust and C# implementations that exercise the Snowflake driver.
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.
So this is strictly an issue with the C Interop layer? There are interop tests in both the Rust and C# implementations that exercise the Snowflake driver.
Yes. Can you force a Snowflake error from the C# interop tests then assert on the vendor code?
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.
Yea, it's strictly an issue with the C interop.
fe717e5 to
97f831f
Compare
|
I applied the fix to my fork, so this will be another issue/PR I procrastinate on because of my own prioritization requirements. I applied the change to the template and re-generated the sources. What is missing (and is more time-consuming) is adding the tests. |
5158d41 to
445ffcc
Compare
445ffcc to
6b3f86a
Compare
|
#1576 is related |
Closes #2800