Oracle connector: Add missing BLOB type mappings
Co-authored-by: Namya Sehgal [email protected]
Description
Previously :
presto> describe oracle.tm_lakehouse_engine.blob_clob;
Column | Type | Extra | Comment
----------+---------+-------+---------
clob_col | varchar | |
(1 row)
Now :
presto> describe oracle.tm_lakehouse_engine.blob_clob;
Column | Type | Extra | Comment
----------+-----------+-------+---------
blob_col | varbinary | |
clob_col | varchar | |
(2 rows)
Motivation and Context
Oracle connector was unable to read native BLOB columns , these columns were completely missing from Presto's metadata view, making it impossible to query tables containing these data types.
Impact
Added proper type mappings to internally convert BLOB types to VARBINARY, enabling read access without introducing first-class BLOB/CLOB support to Presto's type system.
Test Plan
Contributor checklist
- [ ] Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
- [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
- [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality.
- [ ] If release notes are required, they follow the release notes guidelines.
- [ ] Adequate tests were added if applicable.
- [ ] CI passed.
Release Notes
Please follow release notes guidelines and fill in the release notes below.
== RELEASE NOTES ==
Oracle Connector Changes
* Added type mappings to internally convert BLOB types to VARBINARY, enabling read access without introducing first-class BLOB/CLOB support to Presto's type system.
@ethanyzhang imported this issue as lakehouse/presto #25354
Does the Oracle connector doc need a Type Mapping section similar to https://prestodb.io/docs/current/connector/iceberg.html#type-mapping ?
If yes, either you can add it in this PR, or I can open a doc issue to add type mapping section to the Oracle connector doc.
Does the Oracle connector doc need a Type Mapping section similar to https://prestodb.io/docs/current/connector/iceberg.html#type-mapping ?
If yes, either you can add it in this PR, or I can open a doc issue to add type mapping section to the Oracle connector doc.
Hi @steveburnett , I think yes. Mapping BLOB to Varbinary and CLOB to Varchar . Please do let me know where I can add this in the PR.
Does the Oracle connector doc need a Type Mapping section similar to https://prestodb.io/docs/current/connector/iceberg.html#type-mapping ? If yes, either you can add it in this PR, or I can open a doc issue to add type mapping section to the Oracle connector doc.
Hi @steveburnett , I think yes. Mapping BLOB to Varbinary and CLOB to Varchar . Please do let me know where I can add this in the PR.
How about adding a Type Mapping section to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/oracle.rst that is based on the format of https://prestodb.io/docs/current/connector/iceberg.html#type-mapping?
@namya28 Thanks. LGTM. Can you check the maven-checks failure?
Are there any existing product tests that run on jdbc sources? @namya28
Are there any existing product tests that run on jdbc sources? @namya28
There are integration smoke tests for the Oracle connector, which validate that Presto can connect to Oracle via JDBC and run basic queries. However, Oracle is not included in the product test (presto-product-tests), not sure if there will be any licensing restrictions or anything for that.
Does the Oracle connector doc need a Type Mapping section similar to https://prestodb.io/docs/current/connector/iceberg.html#type-mapping ? If yes, either you can add it in this PR, or I can open a doc issue to add type mapping section to the Oracle connector doc.
Hi @steveburnett , I think yes. Mapping BLOB to Varbinary and CLOB to Varchar . Please do let me know where I can add this in the PR.
How about adding a Type Mapping section to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/oracle.rst that is based on the format of https://prestodb.io/docs/current/connector/iceberg.html#type-mapping?
Hi @steveburnett , Thank you for the suggestion! I have pushed the changes. Could you please help review the doc changes.
LGTM! (docs)
Thank you for adding this to the doc!
@steveburnett , Corrected a tiny spelling check and pushed again. Could you please help review again!
Can you add a test?
Hi @rschlussel , The existing tests for Oracle are currently disabled, and I was unable to locate any corresponding product tests. As a result, I performed manual testing, details of which are included in the PR description. Since there is currently no Docker-based setup for running product tests with Oracle, adding tests for this functionality within the existing product test framework might be challenging . I'm happy to explore on re-enabling the existing tests and adding a new test for this specific functionality in a separate PR if possible. Please let me know your thoughts or if you have any preferences regarding this approach.
@namya28 please let's try to re-enable the tests. Unfortunately they've been disabled for what appears to be a trivial reason and we never followed up to re-enable them. But in this project we follow the campsite rule, which is that we leave the codebase better than we found it. Having tests is essential to ongoing maintenance of this connector. Please give it a try and let us know what problems you encounter.
Manually tested the change as given in the PR description as of now . I am working on a separate PR to enable Oracle test classes. I'd like to get this one merged first, and will add the tests in the follow-up.
Reviewer's guide (collapsed on small PRs)
Reviewer's Guide
Adds internal mapping of Oracle BLOB types to Presto VARBINARY and reflects this in the connector documentation.
Entity relationship diagram for updated Oracle BLOB/VARBINARY mapping
erDiagram
ORACLE_TABLE {
clob_col VARCHAR
blob_col BLOB
}
PRESTO_TABLE {
clob_col VARCHAR
blob_col VARBINARY
}
ORACLE_TABLE ||--|| PRESTO_TABLE : "mapped to"
Class diagram for updated OracleClient type mapping
classDiagram
class OracleClient {
+Optional<ReadMapping> toPrestoType(ConnectorSession session, JdbcTypeHandle typeHandle)
}
class ReadMapping
class ConnectorSession
class JdbcTypeHandle {
+int getJdbcType()
}
OracleClient --> ReadMapping
OracleClient --> ConnectorSession
OracleClient --> JdbcTypeHandle
JdbcTypeHandle : Types.CLOB
JdbcTypeHandle : Types.BLOB
OracleClient : varcharReadMapping()
OracleClient : varbinaryReadMapping()
File-Level Changes
| Change | Details | Files |
|---|---|---|
| Map Oracle BLOB columns to VARBINARY in type resolution |
|
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java |
| Document the new BLOB to VARBINARY mapping in Oracle connector docs |
|
presto-docs/src/main/sphinx/connector/oracle.rst |
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai reviewon the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with
@sourcery-ai issueto create an issue from it. - Generate a pull request title: Write
@sourcery-aianywhere in the pull request title to generate a title at any time. You can also comment@sourcery-ai titleon the pull request to (re-)generate the title at any time. - Generate a pull request summary: Write
@sourcery-ai summaryanywhere in the pull request body to generate a PR summary at any time exactly where you want it. You can also comment@sourcery-ai summaryon the pull request to (re-)generate the summary at any time. - Generate reviewer's guide: Comment
@sourcery-ai guideon the pull request to (re-)generate the reviewer's guide at any time. - Resolve all Sourcery comments: Comment
@sourcery-ai resolveon the pull request to resolve all Sourcery comments. Useful if you've already addressed all the comments and don't want to see them anymore. - Dismiss all Sourcery reviews: Comment
@sourcery-ai dismisson the pull request to dismiss all existing Sourcery reviews. Especially useful if you want to start fresh with a new review - don't forget to comment@sourcery-ai reviewto trigger a new review!
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request summary, the reviewer's guide, and others.
- Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.