Skip to content

GCP: Avoid duplicate BigQuery namespace loads - #17871

Open
charliec05 wants to merge 1 commit into
apache:mainfrom
charliec05:agent/bigquery-remove-properties
Open

GCP: Avoid duplicate BigQuery namespace loads#17871
charliec05 wants to merge 1 commit into
apache:mainfrom
charliec05:agent/bigquery-remove-properties

Conversation

@charliec05

Copy link
Copy Markdown
Contributor

Summary

  • delegate namespace existence validation to BigQueryMetastoreClient.removeParameters
  • remove the catalog-level preflight load and empty-set short circuit
  • verify that removing namespace properties performs exactly one client load

This makes removeProperties consistent with setProperties while preserving missing-namespace validation.

Closes #17059

Testing

  • ./gradlew :iceberg-bigquery:test

AI Disclosure

  • Model: GPT-5
  • Platform/Tool: OpenAI Codex
  • Human Oversight: fully reviewed
  • Prompt Summary: A detailed, repository-specific prompt requested removal of the redundant remote lookup, precise call-count coverage, formatting, and full BigQuery module validation.

Delegate namespace validation to removeParameters so removing namespace properties performs a single remote load.

Generated-by: Codex
@github-actions github-actions Bot added the GCP label Aug 28, 2026
Comment on lines 252 to 253
Preconditions.checkNotNull(properties, "Invalid properties to remove: null");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Preconditions.checkNotNull(properties, "Invalid properties to remove: null");
Preconditions.checkNotNull(properties, "Invalid properties to remove: null");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The suggested change looks the same as the original code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(it's an uber nit, about removing the extra newline)

Comment on lines -258 to -260
if (properties.isEmpty()) {
return false;
}

@ebyhr ebyhr Aug 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the motivation to remove this condition? I don't think we need to call BigQueryMetastoreClient#removeParameters when properties is empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GCP: Avoid redundant namespace existence check in removeProperties

3 participants