-
Notifications
You must be signed in to change notification settings - Fork 239
CALCITE-3533 - Map non-jdbc data types to ANY in JdbcSchema #285
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?
CALCITE-3533 - Map non-jdbc data types to ANY in JdbcSchema #285
Conversation
mihaibudiu
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 PR title does not match the issue title.
Also, I find the title confusing: returned by whom?
Maybe you can update the JIRA issue to explain what this achieves.
Is there a test to show what new capabilities this offers?
|
I updated the title and added some tests to cover the functionality. I don't yet have access to Jira to update the issue (I filled out the info and the access is pending). However, the Jira ticket currently does explain what this change is intended to cover. Mapping |
|
These unit tests are not particularly illuminating. |
|
@jeremyosterhoudt would it be possible to fix the check-style of the PR, and also have some end-to-end test showing the fix (as suggested by @mihaibudiu ) ? |
zabetak
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.
It seems that the easier way to showcase the problem (IllegalArgumentException) in a more realistic setting would be through Calcite. @jeremyosterhoudt In addition to the PR here can you come up with a PR in the calcite code base that adds relevant test(s) (possibly in https://github.com/apache/calcite/blob/main/core/src/test/java/org/apache/calcite/test/JdbcTest.java).
In some cases a failure/crash is preferred, compared to returning wrong results or causing data corruption so we should ensure that the fix here will not cause some other undesired side effects.
This PR allows non-standards JDBC/SQL types to be processed as
ANY. Databases such as Oracle and SQLServer (among others) often support SQL types that are not defined as part of the standard Java JDBC type mapping.Currently Avatica (and by proxy Calcite) throw an exception when these types are encountered. However, for many non-standard types this prevents the query from executing successfully even though the results are retrieved and displayed correctly when
ANYis used for these scenarios (SQLSever's DataTimeOffset for example).While this is not a fix per se I do believe this is an improvement over the current behavior. Types could be added directly, however, some types like Oracle's
DateTime With Offsetrequire the Oracle JDBC driver to convert. This would mean including the JDBC driver in Avatica with is likely undesirable. Ideally I think a nice enhancement would be to allow developers/users to specify a non-standard type conversion class/factory that could be called when these types are encountered while embedding any needed libraries (I'm hoping to have a conversation around this and partake in the implementation).