-
Notifications
You must be signed in to change notification settings - Fork 26
Fix driver crash when using INTERVAL types #1085
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?
Fix driver crash when using INTERVAL types #1085
Conversation
Signed-off-by: Roman Dryndik <[email protected]>
Signed-off-by: Roman Dryndik <[email protected]>
|
@samikshya-db could you please give your feedback? Does this PR make sense to you? |
gopalldb
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 change looks good, can you add test cases?
| } else if (columnTypeText.equalsIgnoreCase(VARIANT)) { | ||
| columnTypeName = ColumnInfoTypeName.STRING; | ||
| columnTypeText = VARIANT; | ||
| } else if (INTERVAL_TYPES.contains(columnTypeText.toUpperCase())) { |
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.
can we also say startWith instead of contains? It can then work with any new variant in future as well
| ColumnInfoTypeName.TINYINT, | ||
| ColumnInfoTypeName.BYTE, | ||
| ColumnInfoTypeName.BIGINT)); | ||
| public static final ArrayList<String> INTERVAL_TYPES = |
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.
nit: We can use Set instead of List
Description
DRAFT — need to add tests, requesting early feedback.
This PR fixes a driver crash that occurs when executing a query returning an
INTERVALtype mentioned in #1083 issue.Previously,
PreparedStatement.getMetaData()failed with:This happened because the driver attempted to map interval types to an enum that did not support multi-word SQL type names such as
INTERVAL DAY TO SECOND.What has been done
DatabricksPreparedStatement.getMetaData()and supporting code paths correctly parse and map interval type names.SIGNED_TYPESfrom private to public (just a suggestion — it seems useful and potentially reusable).This PR addresses the issue described in Follow-up #1064 and fixes the metadata retrieval for interval expressions such as:
Testing
Still planning to add automated tests — this is a DRAFT.
Manual testing performed so far:
Used a small Java program (included below) that:
getMetaData()to verify that no exceptions are thrown.getObject()andgetString().Tested on driver version 3.0.4.
Before this fix → driver crashes.
With this PR → metadata is returned successfully and the interval value can be fetched.
How to manually test the change
You can use the following standalone Java snippet to test the fix end-to-end:
Additional Notes to the Reviewer
This PR is a DRAFT — unit tests will be added before marking it ready.
Early feedback is welcome, especially regarding:
SIGNED_TYPESis acceptable.If there are other interval forms worth testing (e.g.,
INTERVAL DAY,INTERVAL SECOND, mixed units), please let me know.I’m open to suggestions on where interval-related tests should live in the test suite.