GCP: Avoid redundant namespace existence check in removeProperties#17061
Open
thswlsqls wants to merge 1 commit into
Open
GCP: Avoid redundant namespace existence check in removeProperties#17061thswlsqls wants to merge 1 commit into
thswlsqls wants to merge 1 commit into
Conversation
BigQueryMetastoreCatalog.removeProperties() checked namespace existence via namespaceExists() (client.load()) before delegating to client.removeParameters(), which performs the same existence check internally (another client.load() call). This doubled the BigQuery API GET requests for a call that setProperties() and the Glue/Hive catalogs perform with a single check. Removing the redundant pre-check makes removeProperties() symmetric with setProperties() and consistent with GlueCatalog/HiveCatalog. Behavior change: calling removeProperties() with an empty properties set on a namespace that does not exist now returns false instead of throwing NoSuchNamespaceException, since the isEmpty() check now runs before any existence check. SupportsNamespaces.removeProperties() documents this exception as optional, so this is not a contract violation. Generated-by: Claude Code
ebyhr
reviewed
Jul 3, 2026
| private BigQueryMetastoreCatalog newCatalog(BigQueryMetastoreClient client) { | ||
| BigQueryMetastoreCatalog catalog = new BigQueryMetastoreCatalog(); | ||
| catalog.setConf(new Configuration()); | ||
| String warehouseLocation = tempFolder.toPath().resolve("hive-warehouse").toString(); |
Member
There was a problem hiding this comment.
hive-warehouse
bigquery-warehouse or just warehouse looks better.
Comment on lines
+78
to
+79
| // client.removeParameters() already checks namespace existence internally, so | ||
| // removeProperties() must not perform a separate existence check of its own. |
Member
There was a problem hiding this comment.
The source of the second removeProperties is unclear. I would rephrase like this:
BigQueryMetastoreClientImpl.removeParameters already checks namespace existence,
so BigQueryMetastoreCatalog.removeProperties should not duplicate that check.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Closes #17059
Summary
BigQueryMetastoreCatalog.removeProperties()checked namespace existence vianamespaceExists()(client.load()) before callingclient.removeParameters(), which already performs the same existence check internally — doubling the BigQuery API GET requests.removeProperties()now delegates directly toclient.removeParameters(), matchingsetProperties()and the parity pattern inGlueCatalog/HiveCatalog, both of which check existence exactly once.removeProperties()with an emptypropertiesset on a namespace that does not exist now returnsfalseinstead of throwingNoSuchNamespaceException, because theisEmpty()check now runs before any existence check.SupportsNamespaces.removeProperties()documents this exception as optional, so this is not a contract violation.Testing done
TestBigQueryMetastoreCatalog#removePropertiesChecksNamespaceExistenceOnlyOnceverifyingclient.load()is called exactly once (via a Mockito spy onFakeBigQueryMetastoreClient).TestBigQueryMetastoreCatalog#removePropertiesPropagatesNoSuchNamespaceExceptionverifying the exception thrown byclient.removeParameters()still propagates.TestBigQueryMetastoreCatalog#removePropertiesReturnsFalseForEmptyPropertiesWithoutCallingClientcovering the documented behavior change../gradlew :iceberg-bigquery:check— 126 tests passed (3 new + 123 existing), 0 failures.