-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Remove foreign handles from materialized view refresh SPI #26454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modifies the materialized view refresh SPI to remove foreign table handles from the connector interface. Instead of passing table handles from other catalogs, the new API provides boolean flags indicating whether foreign sources exist.
- Replace foreign table handle parameters with boolean flags in
beginRefreshMaterializedView()andfinishRefreshMaterializedView() - Filter out foreign table handles in
MetadataManagerand pass only same-catalog handles to connectors - Remove
CatalogHandledependency fromIcebergTableHandleand related components
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java | Updates SPI method signatures to use boolean flags instead of foreign handles |
| core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java | Filters source table handles by catalog and computes boolean flags |
| plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java | Updates method signatures and logic to use boolean flags |
| plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java | Removes CatalogHandle field and related methods |
| plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadataFactory.java | Removes CatalogHandle injection |
| Multiple test files | Updates test code to remove CatalogHandle parameters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
af5756f to
bcc6b2e
Compare
dain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
| computedStatistics, | ||
| sourceTableHandles, | ||
| sourceTableFunctions); | ||
| public Optional<ConnectorOutputMetadata> finishRefreshMaterializedView(ConnectorSession session, ConnectorTableHandle tableHandle, ConnectorInsertTableHandle insertHandle, Collection<Slice> fragments, Collection<ComputedStatistics> computedStatistics, List<ConnectorTableHandle> sourceTableHandles, boolean hasForeignSourceTables, boolean hasSourceTableFunctions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave this chopped down as there are so many args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do that in any of these forwarding classes since the parameter names aren't important to read
...toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java
Show resolved
Hide resolved
| boolean hasForeignSourceTables, | ||
| RetryMode retryMode, | ||
| RefreshType refreshType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider combining hasForeighSourceTable and RefreshType.
bcc6b2e to
0efb9fc
Compare
0efb9fc to
a4c379d
Compare
Release notes
(x) Release notes are required, with the following suggested text: