Introduce reserved-properties setting; reserve "polaris." by default #1417

eric-maynard · 2025-04-21T20:50:38Z

This PR reserves the prefix "polaris." for entity properties and provides a configuration to change the list of reserved prefixes. This is helpful for a few reasons:

It allows the system to set real properties (i.e. setting polaris.internal.owner=eric in a metadata.json) that the user can't modify through the REST API
It allows the system to inject virtual properties (e.g. polaris.virtual.load-duration-ms=1234) into an object that's being loaded, and we can avoid any potential conflict between these virtual properties and real properties
It can prevent unexpected behavior changes when a user's existing conflict becomes a Polaris property. Currently Polaris properties do not follow any kind of structure (e.g. allow.overlapping.table.location) and I plan to fix this in a followup. In the future, these properties can be of a structure like polaris.config.allow.overlapping.table.location and we can allowlist the properties through the reserved-properties filter.

dimas-b

LGTM overall with a bunch of minor comments :)

...rc/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java

dimas-b · 2025-05-08T17:46:50Z

...rc/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java

+    CatalogRole okCatalogRole = new CatalogRole("mycatalogrole", Map.of("foo", "bar"), 0L, 0L, 1);
+    try (Response response =
+        managementApi
+            .request("v1/catalogs/{cat}/catalog-roles", Map.of("cat", catalogName))


Maybe .createCatalogRole(catalogName, okCatalogRole) (add new utility method)?

The reason I didn't do this is that the methods in e.g. ManagementApi have assertThat(response.getStatus()).isEqualTo(CREATED.getStatusCode()) baked into them. The standard in this test and elsewhere seems to be that if you expect a different status code, you don't use a helper method.

See existing test testCreateCatalogWithInvalidName for an example.

...rvice/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java

...kus/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java

service/common/src/main/java/org/apache/polaris/service/config/ReservedProperties.java

dimas-b · 2025-05-08T17:57:54Z

service/common/src/main/java/org/apache/polaris/service/config/ReservedProperties.java

+    for (var entry : updateProperties.entrySet()) {
+      // If a key was removed from the update, we substitute in the existing value as to not remove
+      // it
+      if (!updatePropertiesWithoutReservedProperties.containsKey(entry.getKey())) {


Would it not be simpler to check isReserved(entry.getKey())?

This line might be a little more intuitive, but I'm not sure about simpler. We already did the check in removeReservedProperties, so we don't need to redo it here

dimas-b · 2025-05-08T18:00:27Z

service/common/src/main/java/org/apache/polaris/service/config/ReservedProperties.java

+            throw new IllegalArgumentException(message);
+          } else {
+            LOGGER.debug(message);
+            isReserved = true;


nit: it would be clearer to move this above line 89 and do a break here.

Moved the line, but I think we shouldn't break so that multiple logs can be emitted

Fair enough.

dimas-b · 2025-05-08T18:02:27Z

service/common/src/main/java/org/apache/polaris/service/config/ReservedProperties.java

+    Map<String, String> propertyMap =
+        properties.stream().collect(Collectors.toMap(k -> k, k -> ""));
+    Map<String, String> filteredMap = removeReservedProperties(propertyMap);
+    return filteredMap.keySet().stream().toList();


If we had an isReserved(name) method, we could simply filter the original list.

It's a good idea, but I think we'd still want to delegate to the other removeReservedProperties implementation so that we don't have to duplicate the logic to log/throw

…ties

dimas-b

LGTM, but I think the comment about break in the prefix loop is a good thing to have proactively.

dimas-b · 2025-05-08T23:27:43Z

Let's keep it open for another day or so in case other people have opinions on this change.

singhpk234 · 2025-05-09T22:33:30Z

Awesome @eric-maynard ! I am gonna rebase the rollback compaction pr to this !

* Doc: Add release guide on the website (apache#1539) * main: Update actions/stale digest to f78de97 (apache#1547) * main: Update dependency boto3 to v1.38.12 (apache#1548) * main: Update postgres Docker tag to v17.5 (apache#1549) * main: Update dependency com.adobe.testing:s3mock-testcontainers to v4.2.0 (apache#1551) * main: Update dependency com.nimbusds:nimbus-jose-jwt to v10.3 (apache#1552) * Interface changes for pagination (apache#1528) * add missing apis * more tests, fixes * clean up drop * autolint * changes per review * revert iceberg messages to comply with oss tests * another revert * more iceberg catalog changes * autolint * dependency issues * more wiring * continuing rebase * remaining issues are related to task loading * re-add tests * debugging * fix failing tests * fix another test * changes per review * autolint * some fixes * stable * updates for new persistence * fix * continuing work * more reverts * continue reverts * more reverts * yank tests * autolint * test reverts * try to support limit without real page tokens * autolint * Stable * change comment * autolint * remove catalog config for now * changes per review * more tweaks * simplify types per review * Stable, about to refactor more * re-stable * polish * autolint * more changes per review * stable * Introduce reserved-properties setting; reserve "polaris." by default (apache#1417) * initial commit * initial commit * try to test * quarkus fixes * fix a bunch of callsites * Start applying changes * autolint * chase todos * autolint * bugfix * stable * add one test * stable with more tests * autolint * more tests * autolint * stable tests * clean up * oops * stabilize on main * autolint * more changes per review * Add cleanup support for partition-level statistics files when `DROP TABLE PURGE` (apache#1508) * cleaning up partition stats * update partition stat file extension * update test partition stat write impl * Implement federation to HadoopCatalog (apache#1466) * wip * quarkus fixes * autolint * hadoop impl * autolint * Refactors * refactored * autolint * add config * autolint * stable * Remove breakpoint anchor * add line to application.properties * yank HADOOP * autolint * Spark: Use builder for CreateGenericTableRequest instead of constructor for easier API spec update (apache#1546) * main: Update docker.io/jaegertracing/all-in-one Docker tag to v1.69.0 (apache#1559) * main: Update dependency io.opentelemetry:opentelemetry-bom to v1.50.0 (apache#1558) * main: Update dependency software.amazon.awssdk:bom to v2.31.40 (apache#1567) * main: Update dependency boto3 to v1.38.13 (apache#1556) * Include DISCLAIMER in binary distributions (apache#1568) * NoSQL: merge/rebase-fixes * NoSQL: bump dependencies * fix markdown --------- Co-authored-by: JB Onofré <jbonofre@apache.org> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com> Co-authored-by: danielhumanmod <danieltu.life@gmail.com> Co-authored-by: gh-yzou <167037035+gh-yzou@users.noreply.github.com>

eric-maynard added 4 commits April 21, 2025 13:29

initial commit

5688cfc

initial commit

682ad76

try to test

eea7ad7

quarkus fixes

2c3701f

github-project-automation bot added this to Basic Kanban Board Apr 21, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Apr 21, 2025

eric-maynard added 13 commits April 21, 2025 14:13

fix a bunch of callsites

3d5c230

Start applying changes

c8640aa

autolint

34bd577

chase todos

34c517d

autolint

67b54f2

bugfix

d7d22bf

stable

496afeb

add one test

b3aa47b

stable with more tests

a0458b1

autolint

74d7d73

more tests

c0717fb

autolint

0012ec5

stable tests

bab7c77

eric-maynard marked this pull request as ready for review April 22, 2025 07:16

eric-maynard requested review from adutra, ashvina, dennishuo, dimas-b, jackye1995, jbonofre, vvcephei, collado-mike, snazy and RussellSpitzer as code owners April 22, 2025 07:16

eric-maynard requested review from takidau, MonkeyCanCode, flyrain and ebyhr as code owners April 22, 2025 07:16

eric-maynard added 2 commits April 22, 2025 00:18

clean up

b6c1bdb

oops

e8ecd3a

eric-maynard mentioned this pull request Apr 25, 2025

Feature: Rollback compaction on conflict #1285

Open

pull main

40eac62

eric-maynard requested review from HonahX and pingtimeout as code owners May 8, 2025 05:32

eric-maynard added 2 commits May 7, 2025 22:32

stabilize on main

b128e95

autolint

b20edd1

dimas-b reviewed May 8, 2025

View reviewed changes

eric-maynard added 2 commits May 8, 2025 12:38

Merge branch 'main' of github.com:apache/polaris into reserved-proper…

eceb796

…ties

more changes per review

2030eb5

dimas-b approved these changes May 8, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 8, 2025

eric-maynard merged commit da23e66 into apache:main May 9, 2025
6 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board May 9, 2025

Introduce reserved-properties setting; reserve "polaris." by default #1417

Introduce reserved-properties setting; reserve "polaris." by default #1417

Uh oh!

Conversation

eric-maynard commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b commented May 8, 2025

Uh oh!

Uh oh!

singhpk234 commented May 9, 2025

Uh oh!

Uh oh!

eric-maynard commented Apr 21, 2025 •

edited

Loading

eric-maynard May 8, 2025 •

edited

Loading