Add table procedures support for Lakehouse #26925
Open
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
Iceberg module has several table procedure. Hive and DeltaLake modules have only the "optimize" table procedure. The code for the "optimize" table procedure is identical in all 3 modules. So, the code from Iceberg module is used. We could redeclare the table procedure in the Lakehouse module, or maybe there could be a refactoring so that the identical code for the "optimize" table procedure is placed in a common place and reused from there.
Reviewer's GuideThis PR integrates Iceberg’s table procedures into the Lakehouse connector by registering all procedure providers via a Guice Multibinder, injecting them into the connector, and exposing them through a new getTableProcedures API, along with enabling and testing the optimize procedure across Iceberg, Hive, and Delta Lake modules. Class diagram for LakehouseConnector with table procedures supportclassDiagram
class LakehouseConnector {
- LakehouseLifeCycleManager lifeCycleManager
- LakehouseTransactionManager transactionManager
- LakehouseMetadataFactory metadataFactory
- LakehouseSplitManager splitManager
- LakehouseNodePartitioningProvider nodePartitioningProvider
- LakehouseSessionProperties sessionProperties
- LakehouseTableProperties tableProperties
- IcebergMaterializedViewProperties materializedViewProperties
- Set<TableProcedureMetadata> tableProcedures
+ getTableProcedures() Set<TableProcedureMetadata>
}
class TableProcedureMetadata
LakehouseConnector --> "*" TableProcedureMetadata
Class diagram for LakehouseModule table procedure bindingsclassDiagram
class LakehouseModule {
+ setup(Binder binder)
}
class TableProcedureMetadata
class OptimizeTableProcedure
class OptimizeManifestsTableProcedure
class DropExtendedStatsTableProcedure
class RollbackToSnapshotTableProcedure
class ExpireSnapshotsTableProcedure
class RemoveOrphanFilesTableProcedure
class AddFilesTableProcedure
class AddFilesTableFromTableProcedure
LakehouseModule --> TableProcedureMetadata
TableProcedureMetadata <|-- OptimizeTableProcedure
TableProcedureMetadata <|-- OptimizeManifestsTableProcedure
TableProcedureMetadata <|-- DropExtendedStatsTableProcedure
TableProcedureMetadata <|-- RollbackToSnapshotTableProcedure
TableProcedureMetadata <|-- ExpireSnapshotsTableProcedure
TableProcedureMetadata <|-- RemoveOrphanFilesTableProcedure
TableProcedureMetadata <|-- AddFilesTableProcedure
TableProcedureMetadata <|-- AddFilesTableFromTableProcedure
File-Level Changes
Possibly linked issues
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}
There was a problem hiding this comment.
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> `plugin/trino-lakehouse/src/main/java/io/trino/plugin/lakehouse/LakehouseConnector.java:156-158` </location>
<code_context>
}
+ @Override
+ public Set<TableProcedureMetadata> getTableProcedures()
+ {
+ return tableProcedures;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Returning internal Set directly may expose mutability.
Return an unmodifiable view of tableProcedures to safeguard internal state from external changes.
Suggested implementation:
```java
+ @Override
+ public Set<TableProcedureMetadata> getTableProcedures()
+ {
+ return Collections.unmodifiableSet(tableProcedures);
+ }
+
```
```java
import java.util.Set;
import java.util.Collections;
```
</issue_to_address>
### Comment 2
<location> `plugin/trino-lakehouse/src/test/java/io/trino/plugin/lakehouse/TestLakehouseIcebergConnectorSmokeTest.java:49-50` </location>
<code_context>
)\\E""");
}
+
+ @Test
+ public void testOptimize()
+ {
+ String tableName = "test_optimize_" + randomNameSuffix();
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for invalid procedure arguments and permissions.
Please add tests for invalid argument types, missing required arguments, and permission failures to improve error handling and security coverage.
</issue_to_address>
### Comment 3
<location> `plugin/trino-lakehouse/src/test/java/io/trino/plugin/lakehouse/TestLakehouseHiveConnectorSmokeTest.java:72-73` </location>
<code_context>
)\\E""");
}
+
+ @Test
+ public void testOptimize()
+ {
+ String tableName = "test_optimize_" + randomNameSuffix();
</code_context>
<issue_to_address>
**suggestion (testing):** Add test for optimize procedure with invalid session property value.
Please add a test case for an invalid session property value to verify the connector's handling of unexpected input.
</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
+156
to
+158
| public Set<TableProcedureMetadata> getTableProcedures() | ||
| { | ||
| return tableProcedures; |
There was a problem hiding this comment.
suggestion: Returning internal Set directly may expose mutability.
Return an unmodifiable view of tableProcedures to safeguard internal state from external changes.
Suggested implementation:
+ @Override
+ public Set<TableProcedureMetadata> getTableProcedures()
+ {
+ return Collections.unmodifiableSet(tableProcedures);
+ }
+import java.util.Set;
import java.util.Collections;
Comment on lines
+72
to
+73
| @Test | ||
| public void testOptimize() |
There was a problem hiding this comment.
suggestion (testing): Add test for optimize procedure with invalid session property value.
Please add a test case for an invalid session property value to verify the connector's handling of unexpected input.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Add table procedures support for Lakehouse
Fixes #26754
Description
Iceberg module has several table procedures.
Hive and DeltaLake modules have only the "optimize" table procedure.
The code for the "optimize" table procedure is identical in all 3 modules.
So, the code from Iceberg module is used.
We could redeclare the table procedure in the Lakehouse module, or maybe there could be a refactoring so that the identical code for the "optimize" table procedure is placed in a common place and reused from there.
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(X) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:
Summary by Sourcery
Introduce support for Iceberg-style table procedures in the Lakehouse connector by registering and exposing procedure metadata, and add smoke tests validating these procedures in Iceberg, Hive, and Delta modules.
New Features:
Enhancements:
Tests: