Skip to content

deduplicate storage requests when loading views. #3488

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.

Sign up for GitHub

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

Merged
dimas-b merged 9 commits into from
Jan 23, 2026
Merged

deduplicate storage requests when loading views. #3488

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7d097a1
create file io out side of lambdas
evindj Jan 18, 2026
b3648e2
dedup metadata fetch by reusing ops
evindj Jan 20, 2026
8aa8074
Added unit tests
evindj Jan 21, 2026
a3bb73a
spotless apply
evindj Jan 21, 2026
68e16a0
review comments
evindj Jan 22, 2026
eeaae08
keep inefficient retry for a different PR
evindj Jan 22, 2026
00a5d91
move comment location
evindj Jan 22, 2026
ac443da
cleanup
evindj Jan 22, 2026
3b7ceda
cleanup
evindj Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@
import org.apache.iceberg.util.PropertyUtil;
import org.apache.iceberg.util.Tasks;
import org.apache.iceberg.view.BaseMetastoreViewCatalog;
import org.apache.iceberg.view.BaseView;
import org.apache.iceberg.view.View;
import org.apache.iceberg.view.ViewBuilder;
import org.apache.iceberg.view.ViewMetadata;
import org.apache.iceberg.view.ViewMetadataParser;
Expand Down Expand Up @@ -810,6 +812,24 @@ protected ViewOperations newViewOps(TableIdentifier identifier) {
return new BasePolarisViewOperations(catalogFileIO, identifier);
}

/**
* Override to fix a bug in {@link BaseMetastoreViewCatalog#loadView} where {@link #newViewOps} is

@dimas-b dimas-b Jan 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tangential: Could you also double check for something like this in tables?

@dimas-b dimas-b Jan 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evindj : any insight here? it's not required for current PR, but since you're involved in this issue already, I thought it might be easy for you to quickly double check that 😉

@evindj evindj Jan 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it is my understanding that this would not happen for LoadTable, because loadtable directly uses the operations from the table to construct the response. See https://github.com/apache/polaris/blob/main/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/CatalogHandlerUtils.java#L387

* called twice, causing redundant metadata fetches. This implementation reuses the same {@link
* ViewOperations} instance.
*/
@Override
public View loadView(TableIdentifier identifier) {
if (isValidIdentifier(identifier)) {
ViewOperations ops = newViewOps(identifier);
if (ops.current() == null) {
throw new NoSuchViewException("View does not exist: %s", identifier);
}
return new BaseView(ops, ViewUtil.fullViewName(name(), identifier));
}

throw new NoSuchViewException("Invalid view identifier: %s", identifier);
}

@Override
public boolean dropView(TableIdentifier identifier) {
boolean purge =
Expand Down Expand Up @@ -1395,11 +1415,11 @@ public void doRefresh() {
SHOULD_RETRY_REFRESH_PREDICATE,
getMaxMetadataRefreshRetries(),
metadataLocation -> {
String latestLocationDir =
latestLocation.substring(0, latestLocation.lastIndexOf('/'));
// TODO: Once we have the "current" table properties pulled into the resolvedEntity
// then we should use the actual current table properties for IO refresh here
// instead of the general tableDefaultProperties.
String latestLocationDir =
latestLocation.substring(0, latestLocation.lastIndexOf('/'));

@dimas-b dimas-b Jan 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last thing, please - would you mind removing these changes now that there are not necessary for the purpose of this PR?

FileIO fileIO =
loadFileIOForTableLike(
tableIdentifier,
Expand Down Expand Up @@ -1800,25 +1820,24 @@ public void doRefresh() {
new AttributeMap()
.put(EventAttributes.CATALOG_NAME, catalogName)
.put(EventAttributes.VIEW_IDENTIFIER, identifier)));

refreshFromMetadataLocation(
latestLocation,
SHOULD_RETRY_REFRESH_PREDICATE,
getMaxMetadataRefreshRetries(),
metadataLocation -> {
String latestLocationDir =
latestLocation.substring(0, latestLocation.lastIndexOf('/'));

// TODO: Once we have the "current" table properties pulled into the resolvedEntity
// then we should use the actual current table properties for IO refresh here
// instead of the general tableDefaultProperties.
String latestLocationDir =
latestLocation.substring(0, latestLocation.lastIndexOf('/'));
FileIO fileIO =
loadFileIOForTableLike(
identifier,
Set.of(latestLocationDir),
resolvedEntities,
new HashMap<>(tableDefaultProperties),
Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));

return ViewMetadataParser.read(fileIO.newInputFile(metadataLocation));
});
polarisEventListener.onEvent(
Expand Down