Skip to content

Default PURGE_VIEW_METADATA_ON_DROP to false so views can be dropped out of the box - #5322

Open
ayushtkn wants to merge 1 commit into
apache:mainfrom
ayushtkn:viewPurgeDefault
Open

Default PURGE_VIEW_METADATA_ON_DROP to false so views can be dropped out of the box#5322
ayushtkn wants to merge 1 commit into
apache:mainfrom
ayushtkn:viewPurgeDefault

Conversation

@ayushtkn

Copy link
Copy Markdown
Member

Fixes: #5293

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

Copilot AI lite review requested due to automatic review settings August 18, 2026 11:11
@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Aug 18, 2026

Copilot AI left a comment

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.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Updates the default behavior of view drops by changing PURGE_VIEW_METADATA_ON_DROP to default to false, allowing views to be dropped under default configuration without requiring purge support.

Changes:

  • Change PURGE_VIEW_METADATA_ON_DROP default from true to false and document dependency on DROP_WITH_PURGE_ENABLED.
  • Add an integration test ensuring dropView succeeds when purge-related config is not explicitly set.
  • Document the breaking/default behavior change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
site/content/in-dev/unreleased/configuration/config-sections/flags-polaris_features.md Updates the documented default and adds dependency note, but introduces formatting inconsistencies.
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java Changes the feature flag default and clarifies description text.
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java Adds integration test coverage for dropping views with default purge behavior.
CHANGELOG.md Announces the default change and provides upgrade guidance.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@vigneshio vigneshio left a comment

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.

LGTM

@adutra

adutra commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

This PR changes PURGE_VIEW_METADATA_ON_DROP's default from true to false. This "fixes" the reported inconsistency but creates a new problem: it silently couples two flags that were designed to be independent:

  • DROP_WITH_PURGE_ENABLED is explicitly scoped to tables; its description reads "allows tables to be dropped with the purge parameter set to true."
  • PURGE_VIEW_METADATA_ON_DROP was designed (I guess) as the authoritative, independent control for view purging.

After the PR, view metadata is only purged if the operator sets both DROP_WITH_PURGE_ENABLED=true and PURGE_VIEW_METADATA_ON_DROP=true.

So I'm wondering: isn't the right fix to decouple both flags?

@dimas-b WDYT?

@ayushtkn

Copy link
Copy Markdown
Member Author

Thanx @adutra for the review!!!

This "fixes" the reported inconsistency but creates a new problem: it silently couples two flags that were designed to be independent:

I think they were always coupled, this PR doesn't adds that behaviour there is an existing test explicitily asserting that behaviour linked in the description of the reported issue here:
#5293 (comment)

To be precise this test actually asserts that they are dependent

catalogProps.put(FeatureConfiguration.DROP_WITH_PURGE_ENABLED.catalogConfig(), "false");
catalogProps.put(FeatureConfiguration.PURGE_VIEW_METADATA_ON_DROP.catalogConfig(), "true");
managementApi.updateCatalog(catalog, catalogProps);
assertThatThrownBy(() -> restCatalog.dropView(id)).isInstanceOf(ForbiddenException.class);

I had two solutions as mentioned in the description of the issue that either decouple them and let one deal with only tables or make the defaults sync. @dimas-b suggested to go with default change rather than tweaking the scope of the already existing config here
#5293 (comment)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Views can not be dropped under default configurations

4 participants