test(connector): Enable test class for Oracle connector #25762

namya28 · 2025-08-13T07:21:48Z

Description

This PR involves re-enabling Oracle Test Classes that were previously disabled.

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.

Release Notes

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

== RELEASE NOTES ==

Oracle Connector Changes
* Update Oracle test classes to re-enable them.

...o-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleIntegrationSmokeTest.java

presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleTypes.java

agrawalreetika · 2025-08-14T14:04:31Z

Please squash all your commit into one

agrawalreetika

LGTM

imjalpreet

@namya28, thank you, I have a few comments below.

Also, I don't believe the TestOracleIntegrationSmokeTest and TestOracleDistributedQueries are still enabled. These are excluded from the default profile:

presto/presto-oracle/pom.xml

Lines 233 to 253 in 956978a

    
           <profile> 
        
               <id>default</id> 
        
               <activation> 
        
                   <activeByDefault>true</activeByDefault> 
        
               </activation> 
        
               <build> 
        
                   <plugins> 
        
                       <plugin> 
        
                           <groupId>org.apache.maven.plugins</groupId> 
        
                           <artifactId>maven-surefire-plugin</artifactId> 
        
                           <configuration> 
        
                               <!-- these tests take a very long time so only run them in the CI server --> 
        
                               <excludes> 
        
                                   <exclude>**/TestOracleIntegrationSmokeTest.java</exclude> 
        
                                   <include>**/TestOracleDistributedQueries.java</include> 
        
                               </excludes> 
        
                           </configuration> 
        
                       </plugin> 
        
                   </plugins> 
        
               </build> 
        
           </profile>

They will run if the profile used is ci, as they are not excluded from that:

presto/presto-oracle/pom.xml

Lines 218 to 231 in 956978a

    
           <profile> 
        
               <id>ci</id> 
        
               <build> 
        
                   <plugins> 
        
                       <plugin> 
        
                           <groupId>org.apache.maven.plugins</groupId> 
        
                           <artifactId>maven-surefire-plugin</artifactId> 
        
                           <configuration> 
        
                               <excludes combine.self="override" /> 
        
                           </configuration> 
        
                       </plugin> 
        
                   </plugins> 
        
               </build> 
        
           </profile>

, but I don't see Oracle tests enabled with the ci profile yet.

imjalpreet · 2025-08-18T10:46:11Z

presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleDistributedQueries.java

+        OracleServerTester oracleServerTester = new OracleServerTester();
+        this.queryRunner = createOracleQueryRunner(oracleServerTester, TpchTable.getTables());
+        this.oracleServer = oracleServerTester;


Suggested change

OracleServerTester oracleServerTester = new OracleServerTester();

this.queryRunner = createOracleQueryRunner(oracleServerTester, TpchTable.getTables());

this.oracleServer = oracleServerTester;

this.oracleServer = new OracleServerTester();

this.queryRunner = createOracleQueryRunner(oracleServer, TpchTable.getTables());

imjalpreet · 2025-08-18T10:47:18Z

...o-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleIntegrationSmokeTest.java

+        OracleServerTester oracleServerTester = new OracleServerTester();
+        this.queryRunner = createOracleQueryRunner(oracleServerTester, ORDERS);
+        this.oracleServer = oracleServerTester;


Suggested change

OracleServerTester oracleServerTester = new OracleServerTester();

this.queryRunner = createOracleQueryRunner(oracleServerTester, ORDERS);

this.oracleServer = oracleServerTester;

this.oracleServer = new OracleServerTester();

this.queryRunner = createOracleQueryRunner(oracleServer, ORDERS);

imjalpreet · 2025-08-18T10:50:15Z

presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleTypes.java

+        OracleServerTester oracleServerTester = new OracleServerTester();
+
+        this.queryRunner = createOracleQueryRunner(oracleServerTester);
+        this.oracleServer = oracleServerTester;


Suggested change

OracleServerTester oracleServerTester = new OracleServerTester();

this.queryRunner = createOracleQueryRunner(oracleServerTester);

this.oracleServer = oracleServerTester;

this.oracleServer = new OracleServerTester();

this.queryRunner = createOracleQueryRunner(oracleServer);

imjalpreet · 2025-08-18T10:55:12Z

presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleTypes.java

+        OracleContainer oracleContainer = new OracleContainer("wnameless/oracle-xe-11g-r2");
+        oracleContainer.start();


To clarify, why do we need this only in this test class?

Also, isn't OracleServerTester's constructor already handling this?

tdcmeehan · 2025-09-16T19:47:58Z

@namya28 what is the state of this review?

namya28 · 2025-09-17T07:46:25Z

@namya28 what is the state of this review?

Hi @tdcmeehan , I've addressed the test case failures for the two test classes in this PR. However, the third test class TestOracleDistributedQueries inherits from AbstractTestDistributedQueries class and currently has over 90 test failures after running in CI tests. I’m working through them methodically, but given the volume and complexity, it may take some time to resolve them all.

To keep things clean and avoid premature reviews, I’m considering converting this PR to draft status while I continue working on the remaining test failures. Once I’ve resolved as many as possible and the test suite is in a more stable state, I’ll mark the PR as ready for review again.

Please let me know if this approach works for you, or if you’d suggest an alternative.

Thanks!

sourcery-ai · 2025-11-18T12:14:33Z

Reviewer's Guide

This PR restores disabled Oracle connector tests, adds a full distributed query test suite, enriches OracleClient with additional type mappings and SQL generation, extends the shared test framework with new helpers, and updates CI to include Oracle connector tests.

Class diagram for updated OracleClient type mappings and SQL generation

classDiagram
    class OracleClient {
        +Optional<ReadMapping> toPrestoType(ConnectorSession, JdbcTypeHandle)
        +String toSqlType(Type)
        +String normalizeIdentifier(ConnectorSession, String)
    }
    class ReadMapping
    class ConnectorSession
    class JdbcTypeHandle
    class Type
    class VarcharType {
        +boolean isUnbounded()
        +int getLengthSafe()
    }
    class CharType {
        +int getLength()
    }
    class DecimalType {
        +int getPrecision()
        +int getScale()
    }
    OracleClient --> ReadMapping
    OracleClient --> ConnectorSession
    OracleClient --> JdbcTypeHandle
    OracleClient --> Type
    Type <|-- VarcharType
    Type <|-- CharType
    Type <|-- DecimalType

Class diagram for restored and added Oracle connector test classes

classDiagram
    class TestOracleIntegrationSmokeTest {
        +void testIntegration()
    }
    class TestOracleTypes {
        +void testTypeMappings()
    }
    class TestOracleDistributedQueries {
        +void testDistributedQueries()
    }
    TestOracleIntegrationSmokeTest <|-- TestOracleTypes
    TestOracleIntegrationSmokeTest <|-- TestOracleDistributedQueries

File-Level Changes

Change	Details	Files
Re-enable/refactor Oracle connector integration and types test classes	Renamed disabled test classes and updated constructors Restored @test annotations and session property setups Adjusted expected column definitions in DESCRIBE and type‐roundtrip tests	`presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleIntegrationSmokeTest.java` `presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleTypes.java`
Introduce comprehensive Oracle distributed query tests	Added full TestOracleDistributedQueries suite extending AbstractTestDistributedQueries Configured various system properties (optimizer, timestamp) per test scenario	`presto-oracle/src/test/java/com/facebook/presto/plugin/oracle/TestOracleDistributedQueries.java`
Extend OracleClient type mapping and SQL type generation	Mapped OracleTypes.BINARY_DOUBLE, DATE, TIMESTAMP in toPrestoType Overrode toSqlType to emit VARCHAR2, CHAR, NUMBER(n,m), BINARY_DOUBLE/FLOAT, DATE, TIMESTAMP	`presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java`
Enhance shared test framework utilities	Added assertCreateTableAsSelect overload supporting row count verification and drop Introduced computeExpected(session, sql, resultTypes) helper	`presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueryFramework.java`
Update CI configuration for Oracle connector	Included presto-oracle in the tests workflow matrix	`.github/workflows/tests.yml`

Possibly linked issues

#[Issue ID]: The PR enables the Oracle connector tests in the CI pipeline, directly addressing the issue's goal of enabling container-based tests.

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

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java:312-314` </location>
<code_context>
+        if (isVarcharType(type)) {
+            VarcharType varcharType = (VarcharType) type;
+            if (varcharType.isUnbounded()) {
+                return "VARCHAR2(4000)"; // Oracle max before CLOB
+            }
+            return format("VARCHAR2(%s)", varcharType.getLengthSafe());
</code_context>

<issue_to_address>
**suggestion:** Hardcoding VARCHAR2(4000) for unbounded varchar may not handle larger text fields.

This mapping may cause errors or data loss for inputs exceeding 4000 bytes. Consider documenting the limitation or implementing a fallback to CLOB for unbounded types.

```suggestion
            if (varcharType.isUnbounded()) {
                // Unbounded VARCHAR mapped to CLOB to avoid data loss for inputs exceeding 4000 bytes.
                // See: https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/Character-Data-Types.html
                return "CLOB";
            }
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2025-11-18T12:14:34Z

presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java

+            if (varcharType.isUnbounded()) {
+                return "VARCHAR2(4000)"; // Oracle max before CLOB
+            }


suggestion: Hardcoding VARCHAR2(4000) for unbounded varchar may not handle larger text fields.

This mapping may cause errors or data loss for inputs exceeding 4000 bytes. Consider documenting the limitation or implementing a fallback to CLOB for unbounded types.

Suggested change

if (varcharType.isUnbounded()) {

return "VARCHAR2(4000)"; // Oracle max before CLOB

}

if (varcharType.isUnbounded()) {

// Unbounded VARCHAR mapped to CLOB to avoid data loss for inputs exceeding 4000 bytes.

// See: https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/Character-Data-Types.html

return "CLOB";

}

prestodb-ci added the from:IBM PR from IBM label Aug 13, 2025

namya28 marked this pull request as ready for review August 14, 2025 13:15

namya28 requested a review from a team as a code owner August 14, 2025 13:15

prestodb-ci requested review from a team, sh-shamsan and wanglinsong and removed request for a team August 14, 2025 13:15

namya28 requested a review from agrawalreetika August 14, 2025 13:15

agrawalreetika requested changes Aug 14, 2025

View reviewed changes

tdcmeehan self-assigned this Aug 14, 2025

namya28 force-pushed the enableoracletestclass branch 3 times, most recently from 6642509 to a6d7094 Compare August 14, 2025 14:53

agrawalreetika previously approved these changes Aug 14, 2025

View reviewed changes

imjalpreet requested changes Aug 18, 2025

View reviewed changes

namya28 dismissed agrawalreetika’s stale review via 7ace440 August 21, 2025 06:52

namya28 requested review from czentgr and unidevel as code owners August 21, 2025 06:52

namya28 force-pushed the enableoracletestclass branch 2 times, most recently from aa8b80c to 73cc0ea Compare September 9, 2025 06:38

namya28 marked this pull request as draft September 18, 2025 06:23

namya28 changed the title ~~Enable Oracle Test Class~~ Enable Oracle Test Class [WIP] Sep 18, 2025

This was referenced Sep 23, 2025

feat(plugin-oracle): Implemented getTableStatistics for oracle connector #26120

Merged

Add quotes to column names and disable UpperCase conversion in BaseJdbcClient #25628

Merged

namya28 force-pushed the enableoracletestclass branch from 73cc0ea to a994f0f Compare October 13, 2025 12:27

namya28 mentioned this pull request Oct 13, 2025

feat(Oracle): Add missing BLOB type mappings #25354

Merged

6 tasks

namya28 force-pushed the enableoracletestclass branch from a994f0f to db91645 Compare October 15, 2025 07:11

namya28 force-pushed the enableoracletestclass branch 2 times, most recently from ef3dd50 to 26b44cc Compare November 18, 2025 06:11

namya28 changed the title ~~Enable Oracle Test Class [WIP]~~ test(oracle): Enable test class for Oracle connector Nov 18, 2025

namya28 force-pushed the enableoracletestclass branch from 26b44cc to c919aec Compare November 18, 2025 07:20

namya28 changed the title ~~test(oracle): Enable test class for Oracle connector~~ test(connector): Enable test class for Oracle connector Nov 18, 2025

namya28 marked this pull request as ready for review November 18, 2025 12:13

namya28 requested review from ClarenceThreepwood, elharo, feilong-liu and jaystarshot as code owners November 18, 2025 12:13

prestodb-ci requested a review from a team November 18, 2025 12:13

namya28 requested a review from agrawalreetika November 18, 2025 12:14

sourcery-ai bot reviewed Nov 18, 2025

View reviewed changes

test(oracle): Enable test class for Oracle connector

0c21aa4

namya28 force-pushed the enableoracletestclass branch from e13c84c to 0c21aa4 Compare November 20, 2025 07:27

	<profile>
	<id>default</id>
	<activation>
	<activeByDefault>true</activeByDefault>
	</activation>
	<build>
	<plugins>
	<plugin>
	<groupId>org.apache.maven.plugins</groupId>
	<artifactId>maven-surefire-plugin</artifactId>
	<configuration>
	<!-- these tests take a very long time so only run them in the CI server -->
	<excludes>
	<exclude>**/TestOracleIntegrationSmokeTest.java</exclude>
	<include>**/TestOracleDistributedQueries.java</include>
	</excludes>
	</configuration>
	</plugin>
	</plugins>
	</build>
	</profile>

	<profile>
	<id>ci</id>
	<build>
	<plugins>
	<plugin>
	<groupId>org.apache.maven.plugins</groupId>
	<artifactId>maven-surefire-plugin</artifactId>
	<configuration>
	<excludes combine.self="override" />
	</configuration>
	</plugin>
	</plugins>
	</build>
	</profile>

		OracleContainer oracleContainer = new OracleContainer("wnameless/oracle-xe-11g-r2");
		oracleContainer.start();

test(connector): Enable test class for Oracle connector #25762

Are you sure you want to change the base?

test(connector): Enable test class for Oracle connector #25762

Uh oh!

Conversation

namya28 commented Aug 13, 2025 • edited by agrawalreetika Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agrawalreetika commented Aug 14, 2025

Uh oh!

agrawalreetika left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

imjalpreet Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

imjalpreet Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

imjalpreet Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Sep 16, 2025

Uh oh!

namya28 commented Sep 17, 2025

Uh oh!

sourcery-ai bot commented Nov 18, 2025

Reviewer's Guide

Class diagram for updated OracleClient type mappings and SQL generation

Class diagram for restored and added Oracle connector test classes

File-Level Changes

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

namya28 commented Aug 13, 2025 •

edited by agrawalreetika

Loading