Open
Conversation
Contributor
Reviewer's GuideImplements Oracle connector view support by adding Oracle-specific view metadata operations, delegating view CRUD to OracleClient, and wiring OracleMetadata into the generic JDBC metadata factory while slightly generalizing metadata factory fields. Sequence diagram for Oracle view creation and querying via OracleMetadata and OracleClientsequenceDiagram
actor User
participant PrestoCoordinator
participant JdbcMetadataFactory
participant OracleMetadata
participant OracleClient
participant OracleDB
User->>PrestoCoordinator: CREATE VIEW oracle.schema.view AS SELECT ...
PrestoCoordinator->>JdbcMetadataFactory: create()
JdbcMetadataFactory->>JdbcMetadataFactory: detect OracleClient and instantiate OracleMetadata
JdbcMetadataFactory-->>PrestoCoordinator: OracleMetadata
PrestoCoordinator->>OracleMetadata: createView(session, viewMetadata, viewData, replace)
OracleMetadata->>OracleClient: createView(session, identity, viewName, viewData, replace)
OracleClient->>OracleClient: extractOriginalSql(viewData)
OracleClient->>OracleClient: removeCatalogPrefix(sql)
OracleClient->>OracleDB: CREATE VIEW schema.view AS originalSql
OracleDB-->>OracleClient: success
OracleClient-->>OracleMetadata: return
OracleMetadata-->>PrestoCoordinator: return
User->>PrestoCoordinator: SELECT * FROM oracle.schema.view
PrestoCoordinator->>OracleMetadata: getViews(session, prefix)
OracleMetadata->>OracleClient: listViews(session, identity, Optional(schema))
OracleClient->>OracleDB: JDBC metadata getTables
OracleDB-->>OracleClient: view rows
OracleClient-->>OracleMetadata: List SchemaTableName
OracleMetadata->>OracleClient: getViews(session, identity, viewNames)
OracleClient->>OracleDB: SELECT OWNER, VIEW_NAME, TEXT FROM ALL_VIEWS ...
OracleDB-->>OracleClient: view definition
OracleClient->>OracleDB: JDBC metadata getColumns
OracleDB-->>OracleClient: column metadata
OracleClient-->>OracleMetadata: Map SchemaTableName ConnectorViewDefinition
OracleMetadata-->>PrestoCoordinator: view definitions
PrestoCoordinator-->>User: query results
Class diagram for Oracle view support in OracleClient, OracleMetadata, and JdbcMetadataFactoryclassDiagram
class JdbcClient
class JdbcMetadataCache
class TableLocationProvider
class ConnectorSession
class SchemaTableName
class SchemaTablePrefix
class ConnectorViewDefinition
class ConnectorTableMetadata
class JdbcIdentity
class TypeManager
class OracleClient {
- boolean synonymsEnabled
- int numberDefaultScale
- TypeManager typeManager
+ OracleClient(JdbcConnectorId connectorId, BaseJdbcConfig config, OracleConfig oracleConfig, ConnectionFactory connectionFactory, TypeManager typeManager)
- String[] getTableTypes()
+ String normalizeIdentifier(ConnectorSession session, String identifier)
+ Map~SchemaTableName,ConnectorViewDefinition~ getViews(ConnectorSession session, JdbcIdentity identity, List~SchemaTableName~ tableNames)
+ List~SchemaTableName~ listViews(ConnectorSession session, JdbcIdentity identity, Optional~String~ schema)
+ List~SchemaTableName~ listSchemasForViews(ConnectorSession session, JdbcIdentity identity)
+ void createView(ConnectorSession session, JdbcIdentity identity, SchemaTableName viewName, String viewData, boolean replace)
- String removeCatalogPrefix(String sql)
- String extractOriginalSql(String viewData)
+ void dropView(ConnectorSession session, JdbcIdentity identity, SchemaTableName viewName)
}
class JdbcMetadata {
+ JdbcMetadata(JdbcMetadataCache jdbcMetadataCache, JdbcClient jdbcClient, boolean allowDropTable, TableLocationProvider tableLocationProvider)
+ Map~SchemaTableName,ConnectorViewDefinition~ getViews(ConnectorSession session, SchemaTablePrefix prefix)
+ List~SchemaTableName~ listViews(ConnectorSession session, Optional~String~ schemaName)
+ void createView(ConnectorSession session, ConnectorTableMetadata viewMetadata, String viewData, boolean replace)
+ void dropView(ConnectorSession session, SchemaTableName viewName)
}
class OracleMetadata {
- OracleClient oracleClient
+ OracleMetadata(JdbcMetadataCache jdbcMetadataCache, OracleClient oracleClient, boolean allowDropTable, TableLocationProvider tableLocationProvider)
+ Map~SchemaTableName,ConnectorViewDefinition~ getViews(ConnectorSession session, SchemaTablePrefix prefix)
+ List~SchemaTableName~ listViews(ConnectorSession session, Optional~String~ schemaName)
+ void createView(ConnectorSession session, ConnectorTableMetadata viewMetadata, String viewData, boolean replace)
+ void dropView(ConnectorSession session, SchemaTableName viewName)
}
class JdbcMetadataFactory {
protected JdbcMetadataCache jdbcMetadataCache
protected JdbcClient jdbcClient
protected boolean allowDropTable
protected TableLocationProvider tableLocationProvider
+ JdbcMetadataFactory(JdbcMetadataCache jdbcMetadataCache, JdbcClient jdbcClient, JdbcMetadataConfig config, TableLocationProvider tableLocationProvider)
+ JdbcMetadata create()
protected JdbcMetadata createMetadata()
}
JdbcClient <|-- OracleClient
JdbcMetadata <|-- OracleMetadata
JdbcMetadataFactory --> JdbcClient : uses
JdbcMetadataFactory --> JdbcMetadata : creates
JdbcMetadataFactory --> OracleMetadata : creates via reflection
OracleMetadata --> OracleClient : delegates view operations
JdbcMetadata --> JdbcClient : delegates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
imsayari404
force-pushed
the
oracle_view
branch
from
02aa5b3 to
f081399
Compare
prestodb-ci
requested review from
a team,
ShahimSharafudeen and
namya28
and removed request for
a team
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The Oracle-specific wiring in JdbcMetadataFactory relies on a reflective
clientClassName.contains("OracleClient")check, which is brittle; consider introducing an explicit SPI hook or overridable factory method in the Oracle connector instead of using reflection and string matching on class names. - OracleClient.getViews/createView manually build and parse the view JSON payload (including
extractOriginalSql), duplicating Presto’s view-serialization format and making it fragile to changes; it would be safer to reuse the existing view JSON codec / ConnectorViewDefinition APIs rather than hand-rolled string manipulation. - The
removeCatalogPrefiximplementation uses a simple regex on\b\w+\.(\w+\.\w+)\b, which won’t handle quoted identifiers or more complex SQL correctly and may accidentally rewrite unrelated patterns; consider a more robust approach (e.g., parsing the statement or limiting rewrite to known catalog names) or leaving the SQL unchanged if that’s acceptable for Oracle.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Oracle-specific wiring in JdbcMetadataFactory relies on a reflective `clientClassName.contains("OracleClient")` check, which is brittle; consider introducing an explicit SPI hook or overridable factory method in the Oracle connector instead of using reflection and string matching on class names.
- OracleClient.getViews/createView manually build and parse the view JSON payload (including `extractOriginalSql`), duplicating Presto’s view-serialization format and making it fragile to changes; it would be safer to reuse the existing view JSON codec / ConnectorViewDefinition APIs rather than hand-rolled string manipulation.
- The `removeCatalogPrefix` implementation uses a simple regex on `\b\w+\.(\w+\.\w+)\b`, which won’t handle quoted identifiers or more complex SQL correctly and may accidentally rewrite unrelated patterns; consider a more robust approach (e.g., parsing the statement or limiting rewrite to known catalog names) or leaving the SQL unchanged if that’s acceptable for Oracle.
## Individual Comments
### Comment 1
<location path="presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java" line_range="329-337" />
<code_context>
+ Map<SchemaTableName, ConnectorViewDefinition> views = new HashMap<>();
+
+ // Build the query to fetch view definitions from ALL_VIEWS
+ StringBuilder sql = new StringBuilder(
+ "SELECT OWNER, VIEW_NAME, TEXT FROM ALL_VIEWS WHERE (OWNER, VIEW_NAME) IN (");
+
+ List<String> placeholders = new ArrayList<>();
+ for (int i = 0; i < tableNames.size(); i++) {
+ placeholders.add("(?, ?)");
+ }
+ sql.append(String.join(", ", placeholders));
+ sql.append(")");
+
+ try (PreparedStatement statement = connection.prepareStatement(sql.toString())) {
</code_context>
<issue_to_address>
**suggestion (performance):** Large IN-clause construction may hit Oracle limits and scale poorly.
For workloads with many views this query may fail or degrade significantly. Please either: (a) split the requested views into batches (e.g., 500–1000 pairs per query) and merge the results, or (b) switch to a temp table / join-based approach to avoid very large `IN` lists entirely.
</issue_to_address>
### Comment 2
<location path="presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java" line_range="407-416" />
<code_context>
+
+ String columnsJson = String.join(",", columnJsonList);
+
+ String prestoViewData = String.format(
+ "{\"originalSql\":\"%s\",\"catalog\":\"%s\",\"schema\":\"%s\",\"columns\":[%s],\"owner\":\"%s\",\"runAsInvoker\":false}",
+ escapedSql,
+ connectorId, // Use connector ID as catalog
+ schemaName,
+ columnsJson,
+ owner);
+
+ views.put(viewName, new ConnectorViewDefinition(
+ viewName,
+ Optional.of(owner),
+ prestoViewData));
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Manual JSON construction/parsing for view data is brittle; consider using a JSON library or existing helpers.
This JSON is built via string formatting and ad-hoc escaping, then later parsed via string scanning (`extractOriginalSql`). That approach is brittle for cases like unusual escaping, unicode, or changes to the JSON shape. Prefer constructing and parsing the view JSON with a JSON library or existing Presto `ViewDefinition` helpers instead of manual `replace`/index-based logic.
Suggested implementation:
```java
// Build a structured ConnectorViewDefinition instead of manual JSON
ConnectorViewDefinition viewDefinition = new ConnectorViewDefinition(
oracleViewSql,
Optional.of(connectorId), // Use connector ID as catalog
Optional.of(schemaName),
viewColumns,
Optional.of(owner),
Optional.of(false));
views.put(viewName, viewDefinition);
```
1. Ensure you have a `List<ConnectorViewDefinition.ViewColumn> viewColumns` (or similarly named) built alongside `columnJsonList`. For each column, populate `new ConnectorViewDefinition.ViewColumn(columnName, Optional.of(prestoType.toString()), Optional.empty())` (or the appropriate constructor used elsewhere in the codebase).
2. Remove any now-unused `columnJsonList` construction logic in this method, since the JSON string is no longer needed.
3. Add or verify imports for `com.facebook.presto.spi.ConnectorViewDefinition` and `com.facebook.presto.spi.ConnectorViewDefinition.ViewColumn` as needed.
4. If the actual `ConnectorViewDefinition` constructor signature in your Presto version differs (e.g., different `Optional` parameters or `runAsInvoker` handling), adjust the arguments accordingly to match the existing usage patterns in other connectors (e.g., MySQL, PostgreSQL) in your codebase.
</issue_to_address>
### Comment 3
<location path="presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java" line_range="514-518" />
<code_context>
+ /**
+ * Remove catalog prefix from table references in SQL.
+ */
+ private String removeCatalogPrefix(String sql)
+ {
+ // Pattern to match: word.word.word (catalog.schema.table)
+ // Replace with: word.word (schema.table)
+ return sql.replaceAll("\\b\\w+\\.(\\w+\\.\\w+)\\b", "$1");
+ }
+
</code_context>
<issue_to_address>
**issue:** Regex-based catalog stripping can accidentally modify literals or quoted identifiers.
This pattern will also match three-part identifiers inside string literals or quoted identifiers (e.g., `'a.b.c'`, "A.B.C"), potentially corrupting valid SQL. If catalog stripping is required, consider using an SQL parser to operate on the AST, or at least implement a simple tokenizer so quoted content is skipped before applying the replacement.
</issue_to_address>
### Comment 4
<location path="presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataFactory.java" line_range="40-49" />
<code_context>
+ String clientClassName = jdbcClient.getClass().getName();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Oracle-specific metadata selection via class-name substring and silent reflection failure is brittle.
The current approach relies on `clientClassName.contains(
Suggested implementation:
```java
// Check if this is an Oracle client and create appropriate metadata
String jdbcClientClassName = jdbcClient.getClass().getName();
if ("com.facebook.presto.plugin.oracle.OracleClient".equals(jdbcClientClassName)) {
```
To fully address the brittleness around reflection, you may also want to:
1. Wrap the reflective `Class.forName(...)` and constructor calls in a `catch (ReflectiveOperationException e)` and rethrow as an `IllegalStateException` with a clear message indicating Oracle support is misconfigured, instead of silently falling back to generic `JdbcMetadata`.
2. Optionally log the failure via the existing logging mechanism in this module so operators can diagnose missing Oracle classes at runtime.
These changes would need to be applied around the rest of the `try` block that is not shown in the snippet.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+329
to
+337
| StringBuilder sql = new StringBuilder( | ||
| "SELECT OWNER, VIEW_NAME, TEXT FROM ALL_VIEWS WHERE (OWNER, VIEW_NAME) IN ("); | ||
|
|
||
| List<String> placeholders = new ArrayList<>(); | ||
| for (int i = 0; i < tableNames.size(); i++) { | ||
| placeholders.add("(?, ?)"); | ||
| } | ||
| sql.append(String.join(", ", placeholders)); | ||
| sql.append(")"); |
Contributor
There was a problem hiding this comment.
suggestion (performance): Large IN-clause construction may hit Oracle limits and scale poorly.
For workloads with many views this query may fail or degrade significantly. Please either: (a) split the requested views into batches (e.g., 500–1000 pairs per query) and merge the results, or (b) switch to a temp table / join-based approach to avoid very large IN lists entirely.
Comment on lines
+407
to
+416
| String prestoViewData = String.format( | ||
| "{\"originalSql\":\"%s\",\"catalog\":\"%s\",\"schema\":\"%s\",\"columns\":[%s],\"owner\":\"%s\",\"runAsInvoker\":false}", | ||
| escapedSql, | ||
| connectorId, // Use connector ID as catalog | ||
| schemaName, | ||
| columnsJson, | ||
| owner); | ||
|
|
||
| views.put(viewName, new ConnectorViewDefinition( | ||
| viewName, |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Manual JSON construction/parsing for view data is brittle; consider using a JSON library or existing helpers.
This JSON is built via string formatting and ad-hoc escaping, then later parsed via string scanning (extractOriginalSql). That approach is brittle for cases like unusual escaping, unicode, or changes to the JSON shape. Prefer constructing and parsing the view JSON with a JSON library or existing Presto ViewDefinition helpers instead of manual replace/index-based logic.
Suggested implementation:
// Build a structured ConnectorViewDefinition instead of manual JSON
ConnectorViewDefinition viewDefinition = new ConnectorViewDefinition(
oracleViewSql,
Optional.of(connectorId), // Use connector ID as catalog
Optional.of(schemaName),
viewColumns,
Optional.of(owner),
Optional.of(false));
views.put(viewName, viewDefinition);- Ensure you have a
List<ConnectorViewDefinition.ViewColumn> viewColumns(or similarly named) built alongsidecolumnJsonList. For each column, populatenew ConnectorViewDefinition.ViewColumn(columnName, Optional.of(prestoType.toString()), Optional.empty())(or the appropriate constructor used elsewhere in the codebase). - Remove any now-unused
columnJsonListconstruction logic in this method, since the JSON string is no longer needed. - Add or verify imports for
com.facebook.presto.spi.ConnectorViewDefinitionandcom.facebook.presto.spi.ConnectorViewDefinition.ViewColumnas needed. - If the actual
ConnectorViewDefinitionconstructor signature in your Presto version differs (e.g., differentOptionalparameters orrunAsInvokerhandling), adjust the arguments accordingly to match the existing usage patterns in other connectors (e.g., MySQL, PostgreSQL) in your codebase.
Comment on lines
+514
to
+518
| private String removeCatalogPrefix(String sql) | ||
| { | ||
| // Pattern to match: word.word.word (catalog.schema.table) | ||
| // Replace with: word.word (schema.table) | ||
| return sql.replaceAll("\\b\\w+\\.(\\w+\\.\\w+)\\b", "$1"); |
Contributor
There was a problem hiding this comment.
issue: Regex-based catalog stripping can accidentally modify literals or quoted identifiers.
This pattern will also match three-part identifiers inside string literals or quoted identifiers (e.g., 'a.b.c', "A.B.C"), potentially corrupting valid SQL. If catalog stripping is required, consider using an SQL parser to operate on the AST, or at least implement a simple tokenizer so quoted content is skipped before applying the replacement.
Comment on lines
+40
to
+49
| String clientClassName = jdbcClient.getClass().getName(); | ||
| if (clientClassName.contains("OracleClient")) { | ||
| try { | ||
| // Use reflection to create OracleMetadata | ||
| Class<?> oracleClientClass = Class.forName("com.facebook.presto.plugin.oracle.OracleClient"); | ||
| Class<?> oracleMetadataClass = Class.forName("com.facebook.presto.plugin.oracle.OracleMetadata"); | ||
| if (oracleClientClass.isInstance(jdbcClient)) { | ||
| return (JdbcMetadata) oracleMetadataClass | ||
| .getConstructor( | ||
| JdbcMetadataCache.class, |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Oracle-specific metadata selection via class-name substring and silent reflection failure is brittle.
The current approach relies on `clientClassName.contains(
Suggested implementation:
// Check if this is an Oracle client and create appropriate metadata
String jdbcClientClassName = jdbcClient.getClass().getName();
if ("com.facebook.presto.plugin.oracle.OracleClient".equals(jdbcClientClassName)) {To fully address the brittleness around reflection, you may also want to:
- Wrap the reflective
Class.forName(...)and constructor calls in acatch (ReflectiveOperationException e)and rethrow as anIllegalStateExceptionwith a clear message indicating Oracle support is misconfigured, instead of silently falling back to genericJdbcMetadata. - Optionally log the failure via the existing logging mechanism in this module so operators can diagnose missing Oracle classes at runtime.
These changes would need to be applied around the rest of thetryblock that is not shown in the snippet.
auden-woolfson
requested changes
Mar 16, 2026
Contributor
auden-woolfson
left a comment
auden-woolfson
left a comment
There was a problem hiding this comment.
Can you please add tests?
imsayari404
force-pushed
the
oracle_view
branch
from
f081399 to
8c265d9
Compare
steveburnett
previously approved these changes
Mar 27, 2026
Contributor
steveburnett
left a comment
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Pull branch, local doc build, looks good. Thank you for the documentation!
Contributor
Author
Thank you, @steveburnett, for reviewing. |
imsayari404
force-pushed
the
oracle_view
branch
from
eb5b792 to
9c39e11
Compare
imsayari404
force-pushed
the
oracle_view
branch
from
9c39e11 to
f106c41
Compare
auden-woolfson
requested changes
Mar 31, 2026
| protected final JdbcMetadataCache jdbcMetadataCache; | ||
| protected final JdbcClient jdbcClient; | ||
| protected final boolean allowDropTable; | ||
| protected final TableLocationProvider tableLocationProvider; |
Contributor
There was a problem hiding this comment.
Are these changes still necessary?
Contributor
Author
There was a problem hiding this comment.
I've made the changes
|
|
||
| List<String> placeholders = new ArrayList<>(); | ||
| for (int i = 0; i < tableNames.size(); i++) { | ||
| placeholders.add("(?, ?)"); |
Contributor
There was a problem hiding this comment.
Can we just add the table names directly instead of using placeholders?
Contributor
Author
There was a problem hiding this comment.
I have made the change
| return createMetadata(); | ||
| } | ||
|
|
||
| protected JdbcMetadata createMetadata() |
Contributor
There was a problem hiding this comment.
Please refer to this comment:
imsayari404
force-pushed
the
oracle_view
branch
2 times, most recently
from
ce9c5ca to
60f400b
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 40c1c4e...fcea93a. No notifications. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
added view support for oracle
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Add Oracle-specific metadata and client support to enable full view lifecycle operations in the Oracle connector.
New Features:
Enhancements: