feat(plugin-oracle): Implemented getTableStatistics for oracle connector #26120
+110
−0
Conversation
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
prestodb-ci
requested review from
a team,
NivinCS and
jp-sivaprasad
and removed request for
a team
Contributor
Reviewer's GuideThis PR extends OracleClient by overriding getTableStatistics to fetch and parse table-level and column-level statistics from Oracle’s DBA_TAB_STATISTICS and ALL_TAB_COLUMNS views, converting them into Presto’s TableStatistics and ColumnStatistics via helper methods and enriched logging. Sequence diagram for getTableStatistics data retrieval and conversionsequenceDiagram
participant OC as OracleClient
participant CF as connectionFactory
participant DB as OracleDB
participant TS as TableStatistics
participant CS as ColumnStatistics
OC->>CF: openConnection(JdbcIdentity)
CF->>DB: Connect
OC->>DB: Execute SQL for table stats (DBA_TAB_STATISTICS)
DB-->>OC: Return NUM_ROWS, AVG_ROW_LEN, LAST_ANALYZED
OC->>DB: Execute SQL for column stats (ALL_TAB_COLUMNS)
DB-->>OC: Return column stats
OC->>CS: Build ColumnStatistics for each column
OC->>TS: Build TableStatistics with column stats and row count
OC-->>Caller: Return TableStatistics
Class diagram for updated OracleClient with getTableStatisticsclassDiagram
class OracleClient {
+getTableStatistics(session, handle, columnHandles, tupleDomain) TableStatistics
-getColumnStaticsSql(handle) String
-toDouble(number) double
}
OracleClient --|> BaseJdbcClient
class TableStatistics {
+builder()
+empty()
+setColumnStatistics(columnStatisticsMap)
+setRowCount(rowCount)
}
class ColumnStatistics {
+builder()
+setDataSize(dataSize)
+setNullsFraction(nullsFraction)
+setDistinctValuesCount(distinctValuesCount)
+setRange(DoubleRange)
}
class DoubleRange {
+DoubleRange(low, high)
}
OracleClient --> TableStatistics
OracleClient --> ColumnStatistics
ColumnStatistics --> DoubleRange
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Wrap the PreparedStatement and ResultSet objects in try-with-resources (not just the Connection) to ensure all JDBC resources are properly closed.
- Use SLF4J’s {} placeholders instead of %s in log messages so parameters are substituted correctly.
- Avoid building SQL via String.format with unescaped identifiers—use bind parameters or proper identifier quoting to prevent SQL injection or quoting errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Wrap the PreparedStatement and ResultSet objects in try-with-resources (not just the Connection) to ensure all JDBC resources are properly closed.
- Use SLF4J’s {} placeholders instead of %s in log messages so parameters are substituted correctly.
- Avoid building SQL via String.format with unescaped identifiers—use bind parameters or proper identifier quoting to prevent SQL injection or quoting errors.
## Individual Comments
### Comment 1
<location> `presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java:152-154` </location>
<code_context>
}
+ @Override
+ public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle, List<JdbcColumnHandle> columnHandles, TupleDomain<ColumnHandle> tupleDomain)
+ {
+ try {
+ Preconditions.checkNotNullOrEmpty(handle.getSchemaName(), "schema name");
+ Preconditions.checkNotNullOrEmpty(handle.getTableName(), "table name");
</code_context>
<issue_to_address>
**issue:** Consider handling the case where NUM_ROWS is zero to avoid division by zero.
Guard against numRows being zero when computing nullsFraction to prevent runtime errors and ensure accurate statistics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java
Show resolved
Hide resolved
aaneja
reviewed
Sep 23, 2025
| public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle, List<JdbcColumnHandle> columnHandles, TupleDomain<ColumnHandle> tupleDomain) | ||
| { | ||
| try { | ||
| Preconditions.checkNotNullOrEmpty(handle.getSchemaName(), "schema name"); |
Contributor
There was a problem hiding this comment.
I think a requireNonNull should suffice for this
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java
Outdated
Show resolved
Hide resolved
| "FROM ALL_TAB_COLUMNS\n" + | ||
| "WHERE OWNER = '%s'\n" + | ||
| " AND TABLE_NAME = '%s' AND COLUMN_NAME = '%s') "); | ||
| StringBuffer sqlQueryBuffer = new StringBuffer(); |
Contributor
There was a problem hiding this comment.
nit: Why not StringBuilder ?
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java
Outdated
Show resolved
Hide resolved
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java
Outdated
Show resolved
Hide resolved
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java
Outdated
Show resolved
Hide resolved
Member
|
@ScrapCodes fyi, there is an effort to re-enable Oracle tests here - #25762 |
ScrapCodes
force-pushed
the
oracle_get_stats
branch
from
78b7109 to
79706f4
Compare
ScrapCodes
changed the title
ScrapCodes
force-pushed
the
oracle_get_stats
branch
2 times, most recently
from
314094b to
dca512e
Compare
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Consider using try-with-resources for PreparedStatements and ResultSets to ensure they are always closed and avoid resource leaks.
- Build SQL queries using parameterized statements or proper identifier quoting instead of String.format to prevent SQL injection and handle edge cases.
- Use '{}' placeholders in Logger calls instead of '%s' for consistent formatting with the logging framework.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using try-with-resources for PreparedStatements and ResultSets to ensure they are always closed and avoid resource leaks.
- Build SQL queries using parameterized statements or proper identifier quoting instead of String.format to prevent SQL injection and handle edge cases.
- Use '{}' placeholders in Logger calls instead of '%s' for consistent formatting with the logging framework.
## Individual Comments
### Comment 1
<location> `presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java:150` </location>
<code_context>
}
+ @Override
+ public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle, List<JdbcColumnHandle> columnHandles, TupleDomain<ColumnHandle> tupleDomain)
+ {
+ try {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider closing PreparedStatements explicitly to avoid resource leaks.
PreparedStatements (preparedStatement and preparedStatementCol) should also be closed explicitly to ensure all JDBC resources are released, especially under high query loads.
Suggested implementation:
```java
try (Connection connection = connectionFactory.openConnection(JdbcIdentity.from(session));
PreparedStatement preparedStatement = getPreparedStatement(session, connection, sql)) {
```
If there is another PreparedStatement (e.g., `preparedStatementCol`) created in this method, you should also include it in the try-with-resources statement:
try (Connection connection = ...;
PreparedStatement preparedStatement = ...;
PreparedStatement preparedStatementCol = ...) {
// use preparedStatement and preparedStatementCol
}
If the method creates PreparedStatements in different scopes, ensure each is closed in a try-with-resources or explicitly in a finally block.
</issue_to_address>
### Comment 2
<location> `presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java:174` </location>
<code_context>
+ PreparedStatement preparedStatementCol = getPreparedStatement(session, connection, getColumnStaticsSql(handle));
+ resultSet = preparedStatementCol.executeQuery();
+ Map<ColumnHandle, ColumnStatistics> columnStatisticsMap = new HashMap<>();
+ Map<String, JdbcColumnHandle> columnHandleMap = Maps.uniqueIndex(columnHandles, JdbcColumnHandle::getColumnName);
+ while (resultSet.next() && numRows > 0) {
+ String columnName = resultSet.getString("COLUMN_NAME");
</code_context>
<issue_to_address>
**issue:** Handle missing column names gracefully when populating columnStatisticsMap.
If columnHandleMap.get(columnName) returns null, this null key will be added to columnStatisticsMap, which may cause errors later. Please add a null check and handle such cases appropriately.
</issue_to_address>
### Comment 3
<location> `presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java:239` </location>
<code_context>
+ // varchar.
+ LOG.debug(e, "error while decoding : %s", number);
+ }
+ return NaN;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Use Double.NaN for clarity and consistency.
'NaN' is undefined here; replace it with 'Double.NaN' to prevent compilation errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java
Show resolved
Hide resolved
ScrapCodes
force-pushed
the
oracle_get_stats
branch
from
dca512e to
d1aec45
Compare
aaneja
approved these changes
Oct 14, 2025
ScrapCodes
changed the title
ScrapCodes
changed the title
ScrapCodes
changed the title
ScrapCodes
force-pushed
the
oracle_get_stats
branch
from
d1aec45 to
d9f17eb
Compare
tdcmeehan
changed the title
tdcmeehan
approved these changes
Oct 30, 2025
Contributor
|
@ScrapCodes please fix the release note. |
15 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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
There is a separate PR enabling tests, once that is in - we can work on the tests.
Following unsolved problems exist:
Motivation and Context
Stats can improve the plans involving Oracle connector.
Impact
Test Plan
Contributor checklist