Skip to content

Conversation

@felipecrv
Copy link
Contributor

Closes #2800

@felipecrv felipecrv requested a review from zeroshade as a code owner May 8, 2025 19:34
@felipecrv felipecrv requested review from lidavidm and removed request for zeroshade May 8, 2025 19:34
@github-actions github-actions bot modified the milestone: ADBC Libraries 19 May 8, 2025
@felipecrv felipecrv requested a review from zeroshade May 8, 2025 19:34
// 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)
Copy link
Contributor Author

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.

Copy link
Member

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?)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@felipecrv felipecrv force-pushed the sqlstate-and-vendor branch from fe717e5 to 97f831f Compare May 9, 2025 18:03
@felipecrv
Copy link
Contributor Author

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.

@felipecrv felipecrv force-pushed the sqlstate-and-vendor branch 3 times, most recently from 5158d41 to 445ffcc Compare July 25, 2025 14:40
@felipecrv
Copy link
Contributor Author

felipecrv commented Jul 25, 2025

I've been rebasing this PR with the commits that are part of #3202

(I will stop doing that and wait until we can get #3202 to pass the builds. Ignore compilation errors here and focus on that PR instead for now.)

@felipecrv felipecrv force-pushed the sqlstate-and-vendor branch from 445ffcc to 6b3f86a Compare July 25, 2025 15:38
@zeroshade zeroshade merged commit e227b96 into apache:main Jul 28, 2025
131 of 141 checks passed
@lidavidm
Copy link
Member

#1576 is related

@felipecrv felipecrv deleted the sqlstate-and-vendor branch August 7, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

go: Go drivers don't forward the SQLSTATE and vendor code when they can

4 participants