presto icon indicating copy to clipboard operation
presto copied to clipboard

Oracle connector: Add missing BLOB type mappings

Open namya28 opened this issue 6 months ago • 1 comments

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.

namya28 avatar Jun 18 '25 09:06 namya28

@ethanyzhang imported this issue as lakehouse/presto #25354

prestodb-ci avatar Jun 19 '25 07:06 prestodb-ci

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.

steveburnett avatar Jun 30 '25 13:06 steveburnett

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.

namya28 avatar Jul 01 '25 16:07 namya28

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?

steveburnett avatar Jul 01 '25 16:07 steveburnett

@namya28 Thanks. LGTM. Can you check the maven-checks failure?

nmahadevuni avatar Jul 02 '25 10:07 nmahadevuni

Are there any existing product tests that run on jdbc sources? @namya28

nmahadevuni avatar Jul 02 '25 10:07 nmahadevuni

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.

namya28 avatar Jul 02 '25 13:07 namya28

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.

namya28 avatar Jul 03 '25 15:07 namya28

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!

namya28 avatar Jul 03 '25 15:07 namya28

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 avatar Jul 28 '25 06:07 namya28

@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.

tdcmeehan avatar Aug 04 '25 19:08 tdcmeehan

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.

namya28 avatar Oct 13 '25 15:10 namya28

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
  • Introduce a Types.BLOB case in toPrestoType
  • Return varbinaryReadMapping() for BLOB types
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java
Document the new BLOB to VARBINARY mapping in Oracle connector docs
  • Update schema description example to include blob_col as varbinary
  • Add documentation entry describing BLOB mapping behavior
presto-docs/src/main/sphinx/connector/oracle.rst

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on 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 issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in the pull request body to generate a PR summary at any time exactly where you want it. You can also comment @sourcery-ai summary on the pull request to (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on 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 dismiss on 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 review to 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.

sourcery-ai[bot] avatar Oct 13 '25 15:10 sourcery-ai[bot]