Core: Reuse ops in BaseMetastoreViewCatalog.loadView#17069
Conversation
loadView() created a ViewOperations to check existence via ops.current(), then discarded it and called newViewOps(identifier) again to build the returned BaseView, so every load reloaded view metadata from the metastore twice. BaseViewBuilder.create/replace in the same file, and the parent BaseMetastoreCatalog.loadTable(), all reuse the ops instance used for the existence check. Reuse ops here too, matching that pattern and halving the remote calls per loadView. Generated-by: Claude Code
| import org.apache.iceberg.types.Types; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class TestBaseMetastoreViewCatalogLoadView { |
There was a problem hiding this comment.
I think in this particular case I would probably omit having a separate test class for this case that just verifies that newViewOps() was called a single time. The change seems fairly low-risk, but let's see what other maintainers think
There was a problem hiding this comment.
There's no existing test class for BaseMetastoreViewCatalog itself to fold this into — only the generic ViewCatalogTests shared across catalog implementations, which doesn't fit a single-impl regression check. Happy to move it if you have a preferred home.
| throw new NoSuchViewException("View does not exist: %s", identifier); | ||
| } else { | ||
| return new BaseView(newViewOps(identifier), ViewUtil.fullViewName(name(), identifier)); | ||
| return new BaseView(ops, ViewUtil.fullViewName(name(), identifier)); |
There was a problem hiding this comment.
looks correct to me, thanks for spotting this
|
@thswlsqls the project requires AI disclosure text on the PR itself. Something like |
|
@thswlsqls thanks for your contributions. I noticed that you currently have 32 PRs open, including 14 opened in the last 24 hours. That volume can make it difficult for maintainers to review carefully and can create a lot of noise in the repo. A better way to contribute is to pick one or two focused issues, make sure you understand the changes well enough to explain them end to end, and work with reviewers on those before opening more PRs. This makes it much easier for maintainers to give useful feedback and for your contributions to move forward. As @nastra mentioned, please also review the Iceberg guidelines for AI-assisted contributions: https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions |
|
@nastra @kevinjqliu Fair points, thanks both. I've added an AI Disclosure section to this PR's description and will do the same on my other open PRs. On pace — understood, I'll slow down and focus on getting a smaller set of these through review rather than opening more. |
4 participants
Closes #17068
Summary
loadView()built aViewOperationsto check existence viaops.current(), then discarded it and callednewViewOps(identifier)again to construct the returnedBaseView, reloading view metadata from the metastore twice per call.opsfrom the existence check instead of creating a second instance.BaseViewBuilder.create/replacein the same file and the parentBaseMetastoreCatalog.loadTable(), which already reuseopsthis way.Testing done
TestBaseMetastoreViewCatalogLoadView#loadViewReusesOpsFromExistenceCheck, assertingnewViewOpsis called once (was twice) perloadView()../gradlew :iceberg-core:check— passed (test + spotlessCheck + checkstyle + errorProne)../gradlew :iceberg-core:revapi— passed (backward compatible, no signature change).AI Disclosure