Unblock test createViewWithCustomMetadataLocation
#1320
Merged
Conversation
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
liamzwbao
requested review from
MonkeyCanCode,
RussellSpitzer,
adutra,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
liamzwbao
changed the title
createViewWithCustomMetadataLocation
liamzwbao
force-pushed
the
issue-1273-fix-custom-metadata-location
branch
from
cd7ea05 to
887e001
Compare
liamzwbao
force-pushed
the
issue-1273-fix-custom-metadata-location
branch
from
887e001 to
0dc0fec
Compare
liamzwbao
requested review from
HonahX,
ajantha-bhat,
pingtimeout and
singhpk234
as code owners
dimas-b
previously approved these changes
May 28, 2025
Contributor
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for bringing this up @liamzwbao !
Test code changes LGTM. However, I believe the issues mentioned in the description probably need to be discussed on the dev mailing list rather than in GH. Could you start a discussion thread there?
| String customLocationChild = | ||
| Paths.get(tempDir.toUri().toString(), "custom-location/child").toString(); | ||
|
|
||
| if (requiresNamespaceCreate()) { |
Contributor
There was a problem hiding this comment.
Is this not always true in Polaris?
Contributor
Author
There was a problem hiding this comment.
This is copied from the test in Iceberg and it's unnecessary here, let me remove this check.
| super.createViewWithCustomMetadataLocation(); | ||
| Assertions.assertThatThrownBy(super::createViewWithCustomMetadataLocation) | ||
| .isInstanceOf(ForbiddenException.class) | ||
| .hasMessageContaining("Forbidden: Invalid locations"); |
Contributor
There was a problem hiding this comment.
This change and the following test method LGTM because they assert existing Polaris behaviour.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
dimas-b
approved these changes
May 30, 2025
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
snazy
added a commit
to snazy/polaris
that referenced
this pull request
Jun 13, 2025
* Fix regression test docker setup for purge (apache#1768) * Use canonical catalog property names in tests (apache#1766) * In `PolarisPolicyServiceIntegrationTest` * In `PolarisRestCatalogIntegrationTest` Following up to apache#1557 * Unblock test `createViewWithCustomMetadataLocation` (apache#1320) * Add test for invalid custom metadata location * Add missing properties during table/view creation * main: Update docker.io/prom/prometheus Docker tag to v3.4.1 (apache#1767) * Testing: silence a bunch of harmless test warnings (apache#1773) * `OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended` * Hibernate Validator cannot instrument static methods (`Hibernate Validator does not support constraints on static methods yet. ...`) * ForkJoinPool test lifecycle warning * Couple of split-package warnings * Add unit test for legacy config lookup (apache#1774) Following up on apache#1766 * Handle RequestScoped instance injection gracefully for DefaultConfigurationStore (apache#1758) Although we do a check of !realmContextInstance.isUnsatisfied() it only checks if there is matching bean, however, it doesn't check if the context is active or not. Similarly isResolvable also don't check if the scope is active. Therefore if getConfiguration is called when there is no active scope, it will get Exception like ContextNotActiveException. This actually fails the TaskExecutor because it runs in a separate thread. In order to fix the problem, we introduces a new getConfiguration function to handle the background tasks, and also use isResolvable instead of isUnsatisfied to handle ambiguities. * Restructure the directory and package name for persistence modules (apache#1724) * Re-add missing parameters to create_table python API (apache#1778) It looks like this method lost the `prefix` and `namespace` parameters in apache#1347. They're re-introduced to the spec here, and then I've run: ``` redocly bundle spec/polaris-catalog-service.yaml -o spec/generated/bundled-polaris-catalog-service.yaml ./gradlew regeneratePythonClient ``` Then, some manual reverts: ``` alias gitrevert='git checkout upstream/main --' gitrevert client/python/.github/workflows/python.yml gitrevert client/python/.gitlab-ci.yml gitrevert client/python/pyproject.toml ``` I still hope to automate this process as part of CI soon; see apache#1675 * Replace getConfiguration usage with PolarisCallContext to use RealmContext (PART 1) (apache#1780) * JDBC: Fix getting started config (apache#1781) Fix typo in the JDBC config for getting started, the config should be max_duration_in_ms instead of max_delay_in_ms * main: Pin dependencies (apache#1701) * main: Update dependency pytest to v8 (apache#1710) * main: Update dependency boto3 to v1.38.28 (apache#1777) * Run renovatebot only on the main branch (apache#1786) * INFO: last merged commit a827d26 --------- Co-authored-by: gh-yzou <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Liam Bao <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Prashant Singh <[email protected]> Co-authored-by: JB Onofré <[email protected]>
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.
Summary
Fix #1273
Currently, Polaris does not allow creating a view with a
customMetadataLocationoutside of theallowedLocationsdefined in thestorageConfig, but I think this might be a feature we need to unblock. I've documented Polaris's behavior in this PR and would appreciate the ideas of what should be the expected behavior. Here's a summary:1. View Creation with Custom Metadata Location
A user cannot create a view with a
customMetadataLocationoutside ofallowedLocationsinstorageConfig, unless:write.metadata.path.customMetadataLocationis a subpath of that value.This behavior is enabled by this code block, which updates the storage config to include
write.metadata.path.Example:
If the namespace has
write.metadata.path=file://baseLocation, then the view can be created atfile://baseLocation/customLocation.2. Editing View’s Metadata Location to a Sibling Path
Once the view is created, the user cannot update its
customMetadataLocationto a sibling path likefile://baseLocation/customLocation2Reason:
The validation logic currently checks against the view’s own
write.metadata.pathfor updating, not the parent namespace’s.3. Editing Metadata Location to a Subpath Also Fails
Even updating the metadata location to a child path of the original is not allowed:
file://baseLocation/customLocation→file://baseLocation/customLocation/childReason:
This is rejected because the view’s properties are stored in
internalPropertiesrather thanproperties, so the previous config override becomes ineffective.Questions
write.metadata.pathfeature?