Skip to content

Conversation

@agrawalreetika
Copy link
Member

@agrawalreetika agrawalreetika commented Jan 30, 2026

Description

Motivation and Context

Impact

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.
  • If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ... 

If release note is NOT required, use:

== NO RELEASE NOTE ==

Summary by Sourcery

Extend Oracle connector type mappings and re-enable and expand Oracle integration and distributed tests, including CI coverage.

New Features:

  • Map additional Oracle types (NCLOB, BINARY_DOUBLE, DATE, TIMESTAMP) to appropriate Presto types in the Oracle connector.
  • Override SQL type generation in the Oracle connector for common Presto types such as varchar, char, decimal, numeric primitives, boolean, date, and timestamp.

Enhancements:

  • Introduce a configurable legacy timestamp handling hook in shared query test bases and route selected tests through sessions honoring this configuration.
  • Adjust generic query, integration, and CTAS test utilities to operate with custom sessions and expected results where needed.
  • Refine Oracle varchar and numeric round-trip tests to better align with Oracle limits and behavior.

CI:

  • Add the presto-oracle module to the main tests GitHub Actions workflow with a dedicated CI profile.

Tests:

  • Re-enable and modernize Oracle integration smoke and type tests with self-contained OracleServerTester setup and updated DESCRIBE expectations.
  • Add a new Oracle distributed queries test suite that covers DDL/DML operations, string filtering semantics, CTAS behavior, sharded joins, and large IN predicate handling under Oracle constraints.
  • Update generic query and distributed tests to account for legacy vs non-legacy timestamp semantics and Oracle-specific type limitations.

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Jan 30, 2026
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jan 30, 2026

Reviewer's Guide

Extends the Oracle connector to support additional JDBC/Oracle types and non-legacy timestamp semantics, and wires up a full Oracle test suite (smoke, types, distributed queries) plus CI, updating core tests to allow connectors to opt out of legacy timestamp behavior.

Sequence diagram for Oracle DATE/TIMESTAMP read mapping with OracleClient

sequenceDiagram
    participant User
    participant PrestoCoordinator
    participant OracleClient
    participant OracleJdbcDriver
    participant OracleDatabase

    User->>PrestoCoordinator: submit_query SELECT date_col, ts_col FROM oracle_table
    PrestoCoordinator->>OracleClient: getTableMetadata(session, table)
    OracleClient->>OracleJdbcDriver: getColumns(table)
    OracleJdbcDriver->>OracleDatabase: DESCRIBE oracle_table
    OracleDatabase-->>OracleJdbcDriver: column metadata (DATE, TIMESTAMP)
    OracleJdbcDriver-->>OracleClient: JdbcTypeHandle for DATE, TIMESTAMP

    OracleClient->>OracleClient: toPrestoType(session, typeHandle)
    OracleClient->>OracleClient: switch typeHandle.getJdbcType()
    OracleClient-->>PrestoCoordinator: ReadMapping using timestampReadMapping for DATE and TIMESTAMP

    PrestoCoordinator->>OracleJdbcDriver: executeQuery with non_legacy_timestamp_mappings
    OracleJdbcDriver->>OracleDatabase: SELECT date_col, ts_col FROM oracle_table
    OracleDatabase-->>OracleJdbcDriver: rows with DATE and TIMESTAMP values
    OracleJdbcDriver-->>PrestoCoordinator: JDBC rows
    PrestoCoordinator-->>User: result set with Presto TIMESTAMP columns
Loading

Sequence diagram for OracleClient toSqlType when creating a table

sequenceDiagram
    participant User
    participant PrestoCoordinator
    participant OracleClient
    participant OracleJdbcDriver
    participant OracleDatabase

    User->>PrestoCoordinator: CREATE TABLE oracle_table(a VARCHAR(2000), b DOUBLE, c TIMESTAMP)
    PrestoCoordinator->>OracleClient: toSqlType(VarcharType length=2000)
    OracleClient->>OracleClient: isVarcharType(type) and getLengthSafe()
    OracleClient-->>PrestoCoordinator: NCLOB

    PrestoCoordinator->>OracleClient: toSqlType(DOUBLE)
    OracleClient-->>PrestoCoordinator: BINARY_DOUBLE

    PrestoCoordinator->>OracleClient: toSqlType(TIMESTAMP)
    OracleClient-->>PrestoCoordinator: TIMESTAMP

    PrestoCoordinator->>OracleJdbcDriver: CREATE TABLE oracle_table(a NCLOB, b BINARY_DOUBLE, c TIMESTAMP)
    OracleJdbcDriver->>OracleDatabase: execute DDL
    OracleDatabase-->>OracleJdbcDriver: table created
    OracleJdbcDriver-->>PrestoCoordinator: success
    PrestoCoordinator-->>User: CREATE TABLE succeeded
Loading

Class diagram for updated OracleClient type mappings

classDiagram
    class OracleClient {
        +Optional~ReadMapping~ toPrestoType(ConnectorSession session, JdbcTypeHandle typeHandle)
        +String toSqlType(Type type)
    }

    class BaseJdbcClient {
        +Optional~ReadMapping~ toPrestoType(ConnectorSession session, JdbcTypeHandle typeHandle)
        +String toSqlType(Type type)
    }

    class Type
    class VarcharType {
        +boolean isUnbounded()
        +int getLengthSafe()
    }
    class CharType {
        +int getLength()
    }
    class DecimalType {
        +int getPrecision()
        +int getScale()
    }

    class ReadMapping
    class ConnectorSession
    class JdbcTypeHandle {
        +int getJdbcType()
        +int getColumnSize()
    }

    OracleClient --|> BaseJdbcClient
    OracleClient ..> VarcharType : uses
    OracleClient ..> CharType : uses
    OracleClient ..> DecimalType : uses
    OracleClient ..> Type : uses
    OracleClient ..> ReadMapping : returns
    OracleClient ..> JdbcTypeHandle : uses
    OracleClient ..> ConnectorSession : uses

    %% Highlight key JDBC and Presto type relationships
    class JdbcTypes {
        +int CLOB
        +int NCLOB
        +int BLOB
        +int TINYINT
        +int FLOAT
        +int DOUBLE
        +int REAL
        +int NUMERIC
        +int DECIMAL
        +int CHAR
        +int DATE
        +int TIMESTAMP
        +int VARCHAR
    }

    class OracleTypes {
        +int BINARY_DOUBLE
        +int BINARY_FLOAT
    }

    OracleClient ..> JdbcTypes : inspects
    OracleClient ..> OracleTypes : inspects

    class PrestoTypes {
        +Type BIGINT
        +Type INTEGER
        +Type DOUBLE
        +Type REAL
        +Type BOOLEAN
        +Type DATE
        +Type TIMESTAMP
    }

    OracleClient ..> PrestoTypes : maps from
Loading

File-Level Changes

Change Details Files
Broaden Oracle type mappings for reading from and writing to Oracle, including CLOB/NCLOB, binary floats/doubles, and DATE/TIMESTAMP handling.
  • Map JDBC NCLOB to unbounded varchar when reading from Oracle.
  • Map Oracle BINARY_DOUBLE to Presto DOUBLE when reading.
  • Map JDBC DATE and TIMESTAMP to Presto TIMESTAMP with an inline comment explaining Oracle storage semantics and linking to docs.
  • Override toSqlType to emit NCLOB for large/unbounded varchar, CLOB for large CHAR, NUMBER for DECIMAL/BIGINT/INTEGER/BOOLEAN, BINARY_DOUBLE/BINARY_FLOAT for DOUBLE/REAL, and DATE/TIMESTAMP for the corresponding Presto types, delegating to the superclass otherwise.
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java
Introduce connector-specific control over legacy timestamp behavior and use it in generic tests to make them work with non-legacy connectors such as Oracle.
  • Add isLegacyTimestampEnabled hook and sessionWithLegacyTimestamp helper in AbstractTestQueries to construct sessions with LEGACY_TIMESTAMP toggled per connector.
  • Switch selected tests in AbstractTestQueries to use a session obtained via sessionWithLegacyTimestamp when querying orders or performing UNION/aggregation queries that are sensitive to timestamp semantics.
  • Add isLegacyTimestampEnabled override in AbstractTestIntegrationSmokeTest and guard several generic tests so they only run when legacy timestamp is enabled.
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestIntegrationSmokeTest.java
Extend the generic test framework with helpers that accept Session and expected row count/result, enabling more flexible CTAS and expected-result computation for connectors.
  • Add assertCreateTableAsSelect overload that accepts a Session plus explicit row-count query and ensures the table is dropped and absent after the test.
  • Add computeExpected overload that takes a Session so expected queries can be executed with non-default system properties.
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueryFramework.java
Enable and modernize the Oracle integration smoke and type tests, previously disabled due to TestNG constructor issues, and adjust expectations to the new type mapping behavior.
  • Rename DisabledTestOracleIntegrationSmokeTest to TestOracleIntegrationSmokeTest, switch to a no-arg constructor that internally instantiates OracleServerTester, and wire createOracleQueryRunner accordingly.
  • Override isLegacyTimestampEnabled in the Oracle smoke test to return false, so the shared test base can adjust behavior for non-legacy timestamps.
  • Update the DESCRIBE orders test to expect additional metadata columns (precision/scale/length) and BIGINT-typed sizes instead of simple four-column output.
  • Rename DisabledTestOracleTypes to TestOracleTypes, give it a no-arg constructor that creates OracleServerTester internally, and simplify/adjust tests: trim overly precise NUMBER insertion to a supported scale and adjust varchar round-trip tests to use unbounded/4000-length variants consistent with new type mapping.
presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleIntegrationSmokeTest.java
presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleTypes.java
Add a comprehensive Oracle distributed queries test suite tailored to connector limitations (no deletes, no non-autocommit writes, limited types, non-legacy timestamps) and adjust generic distributed tests to cooperate with it.
  • Introduce TestOracleDistributedQueries extending AbstractTestDistributedQueries, constructing an OracleServerTester-backed QueryRunner with full TPC-H tables.
  • Override capability methods (supportsViews, isLegacyTimestampEnabled, testDelete, transaction tests, update tests, payload join/subfield tests) to skip unsupported operations and types for Oracle.
  • Customize a wide range of distributed tests (large IN, CREATE TABLE/CTAS, symbol aliasing, rename table/column, add column, insert, string filters, large IN with histograms, sharded join optimization) to avoid Oracle limitations such as max 30-character identifiers, IN list length limits, unsupported array/map/row types, and CHAR/VARCHAR behavior, frequently using explicit Sessions with LEGACY_TIMESTAMP=false and REDISTRIBUTE_WRITES=false.
  • Adjust expectations in SHOW COLUMNS and DESCRIBE-related tests to match parametrized varchar and numeric metadata (precision/scale/length) returned by the Oracle connector.
presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleDistributedQueries.java
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java
Wire Oracle tests into CI so the connector is exercised as part of the standard workflow.
  • Add :presto-oracle -P ci to the Maven matrix in the tests GitHub Actions workflow so Oracle tests run under the ci profile.
.github/workflows/tests.yml

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:IBM PR from IBM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants