Add location properties from TableMetadata into Table entity internalProps by singhpk234 · Pull Request #3226 · apache/polaris

singhpk234 · 2025-12-06T17:42:47Z

About the change

Presently since not all the location properties are stored in the entity one has to do complete loadTable table even for existing use cases such as credential endpoint which in present scenario since polaris doesn't store the complete metadata copy inside the persistence is kind of expensive.

With this we will have a way to do cred vending, in future (to remote sign) without going to object store.
Note if we take dependency on this we would have to think of backfill but for step 1 it seems reasonable step, would love to know what other folks think

polaris/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

Lines 616 to 625 in 8ba112e

    
                 catalog.loadTableWithAccessDelegation( 
        
                     tableIdentifier, 
        
                     "all", 
        
                     Optional.of(new PolarisResourcePaths(prefix).credentialsPath(tableIdentifier))); 
        
             return Response.ok( 
        
                     ImmutableLoadCredentialsResponse.builder() 
        
                         .credentials(loadTableResponse.credentials()) 
        
                         .build()) 
        
                 .build(); 
        
           });

co-author : @adutra

Checklist

🛡️ Don't disclose security issues! (contact [email protected])
🔗 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)

…roperties

adnanhemani

Looks like a reasonable change to me - I don't see any reason why storing these storage locations is a hefty and/or unreasonable addition to the entities. Happy to switch my opinion if anyone has valid concerns regarding the addition of these locations.

snazy

This change raises a question:
Does Polaris actively support setting these properties at all?
Polaris performs a lot of effort to ensure that table/view/namespace locations do not overlap and conflict with each other.
Seeing this change, I realize that neither of these two properties is subject to the same location-overlap or location-validity checks.

I think this change deserves a broader dev mailing list discussion about these two properties in general, mentioning that these properties (and their deprecated, but still evaluated older properties) can break both location validity and overlap checks and assumptions.

dimas-b

The diff in this PR LGTM in isolation.

However, making use of the new entity properties can be tricky as Polaris code will have to deal with old entities (on upgrade), which may not have the new properties.

So all in all, having a dev discussion could be useful, indeed.

adutra · 2025-12-08T18:59:15Z

I might be wrong but it seems the locations are taken into account for overlap in BasePolarisTableOperations#doCommit:

polaris/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

Lines 1465 to 1472 in fe57ea1

    
           Set<String> dataLocations = 
        
               StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties()); 
        
           CatalogUtils.validateLocationsForTableLike( 
        
               realmConfig, tableIdentifier, dataLocations, resolvedStorageEntity); 
        
           // also validate that the table location doesn't overlap an existing table 
        
           dataLocations.forEach( 
        
               location -> 
        
                   validateNoLocationOverlap(

The problem is that later on in that same method, the internal properties map is computed:

polaris/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

Line 1506 in fe57ea1

    
           Map<String, String> storedProperties = buildTableMetadataPropertiesMap(metadata);

But the map currently doesn't include write.data.path or write.metadata.path:

polaris/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

Lines 1658 to 1702 in fe57ea1

    
           private static Map<String, String> buildTableMetadataPropertiesMap(TableMetadata metadata) { 
        
             Map<String, String> storedProperties = new HashMap<>(); 
        
             // Location specific properties 
        
             storedProperties.put(IcebergTableLikeEntity.LOCATION, metadata.location()); 
        
             if (metadata.properties().containsKey(TableProperties.WRITE_DATA_LOCATION)) { 
        
               storedProperties.put( 
        
                   IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, 
        
                   metadata.properties().get(TableProperties.WRITE_DATA_LOCATION)); 
        
             } 
        
             if (metadata.properties().containsKey(TableProperties.WRITE_METADATA_LOCATION)) { 
        
               storedProperties.put( 
        
                   IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, 
        
                   metadata.properties().get(TableProperties.WRITE_METADATA_LOCATION)); 
        
             } 
        
             storedProperties.put( 
        
                 IcebergTableLikeEntity.FORMAT_VERSION, String.valueOf(metadata.formatVersion())); 
        
             storedProperties.put(IcebergTableLikeEntity.TABLE_UUID, metadata.uuid()); 
        
             storedProperties.put( 
        
                 IcebergTableLikeEntity.CURRENT_SCHEMA_ID, String.valueOf(metadata.currentSchemaId())); 
        
             if (metadata.currentSnapshot() != null) { 
        
               storedProperties.put( 
        
                   IcebergTableLikeEntity.CURRENT_SNAPSHOT_ID, 
        
                   String.valueOf(metadata.currentSnapshot().snapshotId())); 
        
             } 
        
             storedProperties.put( 
        
                 IcebergTableLikeEntity.LAST_COLUMN_ID, String.valueOf(metadata.lastColumnId())); 
        
             storedProperties.put(IcebergTableLikeEntity.NEXT_ROW_ID, String.valueOf(metadata.nextRowId())); 
        
             storedProperties.put( 
        
                 IcebergTableLikeEntity.LAST_SEQUENCE_NUMBER, String.valueOf(metadata.lastSequenceNumber())); 
        
             storedProperties.put( 
        
                 IcebergTableLikeEntity.LAST_UPDATED_MILLIS, String.valueOf(metadata.lastUpdatedMillis())); 
        
             if (metadata.sortOrder() != null) { 
        
               storedProperties.put( 
        
                   IcebergTableLikeEntity.DEFAULT_SORT_ORDER_ID, 
        
                   String.valueOf(metadata.defaultSortOrderId())); 
        
             } 
        
             if (metadata.spec() != null) { 
        
               storedProperties.put( 
        
                   IcebergTableLikeEntity.DEFAULT_SPEC_ID, String.valueOf(metadata.defaultSpecId())); 
        
               storedProperties.put( 
        
                   IcebergTableLikeEntity.LAST_PARTITION_ID, 
        
                   String.valueOf(metadata.lastAssignedPartitionId())); 
        
             } 
        
             return storedProperties; 
        
           }

So, the only way to check those locations is to load the table from the object store.

adutra · 2025-12-08T19:00:23Z

However, making use of the new entity properties can be tricky as Polaris code will have to deal with old entities (on upgrade), which may not have the new properties.

Correct – code would have to be lenient to the properties being absent. For request signing at least, the idea would be to load the table if the properties are absent, then update the entity, so that next time the properties are there.

singhpk234 · 2025-12-09T06:38:37Z

So, the only way to check those locations is to load the table from the object store.

precisely, the idea is to have location props in sync with what is present in the object store, to avoid redundant call as presently the loadCreds is doing unneccessary call to load from objectstore.
I see this PR atleast (which is first step to what i wanna achieve) as an extension to Mike's recent PR my POV

Add properties from TableMetadata into Table entity internalProperties #2735

when we plan to use it we need to handle backfill, Here is the discuss in Alex on the same

Add support for S3 request signing #2280 (comment)

Re: opening mail list thread, sure happy to facilitate the discussion over MT on how to handle back fill, me and Alex had some idea which Alex mentioned, on this context but defintely more inputs are certainly welcomed.

github-actions · 2026-01-10T02:12:56Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

adutra

I think this change can be useful, even if there is no call site today in Polaris that could benefit from the new entity properties.

dimas-b

This change LGTM purely from the java code perspective and I do not mind merging it.

TBH, I lost context with any related dev discussions. @singhpk234 : Could you update on the status?

From my POV, I do not require a dev discussion, but it might be worth opening for the sake of awareness of other Polaris developers.

dimas-b · 2026-02-03T01:01:06Z

Related dev discussion: https://lists.apache.org/thread/5obdjwhpk7y6kxkqst4m47pocr4k2fk3

singhpk234 · 2026-02-09T23:57:59Z

Thanks for the review everyone, i merged this since there were no objection, i will followup with the credential endpoint optimisation !

* "Stale" job: don't close stale issues (apache#3683) This PR also fixes the configuration and upgrades the action to the latest version. * Fix integration-tests after apache#3704 (apache#3711) Short story: This change fixes the int-tests in `:polaris-runtime-service`. Long story is this: In `:polaris-runtime-service` we intentionally declare "just" a `platform(libs.quarkus.bom)` dependency, because of the `:polaris-spark-integration-*` module. It is however generally recommended by Quarkus to _only_ use `enforcedPlatform(libs.quarkus.bom)` to effectively prevent breaking changes coming from transitive dependencies. Which is exactly what happend after apache#3704. Why did CI not catch this issue? The answer is pretty simple: The effective Gradle task inputs, including the `intTestRuntimeClasspath` did not change. So the previously cached test outcomes could be reused, and the int-tests did not run. Just adding the `runtimeClasspath` as another task-input of the `intTest` may _not_ work as intended. Why does _removing_ `implementation(platform(libs.opentelemetry.bom))` help? Simply because that lets the dependencies fall back to to the declared transitive dependencies. We do not have direct dependencies to OTel. The correct fix here _would_ be to use `implementation(enforcedPlatform(libs.quarkus.bom))`, but that breaks the Spark plugin integration tests. There is a better alternative: Let the Spark plugin tests leverage polaris-apprunner, which is meant for exactly the use case of effectively decoupling some module from the build requirements of a Quarkus application. * CI: fix missing Gradle build scans (apache#3709) Publishing the Gradle build scans requires the `DEVELOCITY_ACCESS_KEY` secret. For workflow-calls, each secret to be propagated to a called workflow must be explicitly defined. This change propagates the secrets from `ci-main.yml` to the called `ci.yml`. * Add Polaris Community Meetings for 2026-01-22 and 2026-02-05 (apache#3715) * Add location propertiesfrom TableMetadata into Table entity internalProperties (apache#3226) * Compile client modules with Java 17 compatibility (apache#3712) * Last merged commit ba048c8 --------- Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Prashant Singh <[email protected]>

Add location propertiesfrom TableMetadata into Table entity internalP…

fe57ea1

…roperties

github-project-automation bot added this to Basic Kanban Board Dec 6, 2025

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

singhpk234 requested review from adutra, dimas-b and flyrain December 6, 2025 17:45

adnanhemani approved these changes Dec 8, 2025

View reviewed changes

snazy reviewed Dec 8, 2025

View reviewed changes

dimas-b reviewed Dec 8, 2025

View reviewed changes

github-actions bot added the stale label Jan 10, 2026

github-actions bot closed this Jan 17, 2026

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Jan 17, 2026

singhpk234 reopened this Jan 27, 2026

github-project-automation bot moved this from Done to PRs In Progress in Basic Kanban Board Jan 27, 2026

github-actions bot removed the stale label Jan 27, 2026

adutra approved these changes Jan 27, 2026

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jan 27, 2026

dimas-b approved these changes Jan 31, 2026

View reviewed changes

flyrain approved these changes Feb 3, 2026

View reviewed changes

singhpk234 closed this Feb 7, 2026

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Feb 7, 2026

singhpk234 reopened this Feb 7, 2026

github-project-automation bot moved this from Done to PRs In Progress in Basic Kanban Board Feb 7, 2026

singhpk234 merged commit de8cadf into apache:main Feb 9, 2026
45 checks passed

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Feb 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add location properties from TableMetadata into Table entity internalProps#3226

Add location properties from TableMetadata into Table entity internalProps#3226
singhpk234 merged 1 commit intoapache:mainfrom
singhpk234:feature/optimize-cred-endpount

singhpk234 commented Dec 6, 2025 •

edited

Loading

Uh oh!

adnanhemani left a comment •

edited

Loading

Uh oh!

snazy left a comment

Uh oh!

dimas-b left a comment

Uh oh!

adutra commented Dec 8, 2025

Uh oh!

adutra commented Dec 8, 2025

Uh oh!

singhpk234 commented Dec 9, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jan 10, 2026

Uh oh!

adutra left a comment

Uh oh!

dimas-b left a comment •

edited

Loading

Uh oh!

dimas-b commented Feb 3, 2026

Uh oh!

Uh oh!

singhpk234 commented Feb 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	catalog.loadTableWithAccessDelegation(
	tableIdentifier,
	"all",
	Optional.of(new PolarisResourcePaths(prefix).credentialsPath(tableIdentifier)));
	return Response.ok(
	ImmutableLoadCredentialsResponse.builder()
	.credentials(loadTableResponse.credentials())
	.build())
	.build();
	});

Conversation

singhpk234 commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About the change

Checklist

Uh oh!

adnanhemani left a comment • 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!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

adutra commented Dec 8, 2025

Uh oh!

adutra commented Dec 8, 2025

Uh oh!

singhpk234 commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 10, 2026

Uh oh!

adutra left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment • 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!

dimas-b commented Feb 3, 2026

Uh oh!

Uh oh!

singhpk234 commented Feb 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

singhpk234 commented Dec 6, 2025 •

edited

Loading

adnanhemani left a comment •

edited

Loading

singhpk234 commented Dec 9, 2025 •

edited

Loading

dimas-b left a comment •

edited

Loading