-
Notifications
You must be signed in to change notification settings - Fork 853
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
Add CommandGetXdbcTypeInfo to Flight SQL Server #4055
Conversation
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 sensible to me, but I'm not very familiar with this area of the codebase. Perhaps @alamb or @avantgardnerio might be able to take look
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.
Add CommandGetXdbcTypeInfo in a few moer places
Before we merge, I noticed that the same method is also missing for the client. I am adding it as well. |
@alamb / @avantgardnerio : Should I add a new PR or do you prefer to extend this? Edit: NVM, I had a buggy ODBC driver. CommandStatementQuery should never be in a do_get. |
Actually, this deserves a separate issue. |
This is why I think integration testing is almost as important as manually trying to interpret the spec. IMO we should try stuff with the JDBC / ODBC drivers, record what happens, then try to mimic those tests with the Rust client and add them to the test suite. I wouldn't want to merge the change that breaks something about JDBC that others are actively using. In the case of |
I agree -- I think this testing is covered by #2409. I agree it would be a fantastic addition |
Which issue does this PR close?
Closes #4054