Skip to content

Add properties from TableMetadata into Table entity internalProperties #2735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
collado-mike merged 2 commits into from
Oct 23, 2025
Merged

Add properties from TableMetadata into Table entity internalProperties #2735

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dfe642
Add properties from TableMetadata into Table entity internalProperties
collado-mike Sep 30, 2025
e36e7eb
Made table properties constants and pulled out static utility method
collado-mike Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ public B addProperty(String key, String value) {
return (B) this;
}

public B setInternalProperties(Map<String, String> internalProperties) {
public B setInternalProperties(@Nonnull Map<String, String> internalProperties) {
this.internalProperties = new HashMap<>(internalProperties);
return (B) this;
}
Expand Down
47 changes: 46 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/table/IcebergTableLikeEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.google.common.base.Preconditions;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.Optional;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.rest.RESTUtil;
Expand All @@ -32,6 +32,10 @@
import org.apache.polaris.core.entity.PolarisEntitySubType;
import org.apache.polaris.core.entity.PolarisEntityType;

import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

/**
* An entity type for {@link TableLikeEntity} instances that conform to iceberg semantics around
* locations. This includes both Iceberg tables and Iceberg views.
Expand Down Expand Up @@ -83,6 +87,24 @@ public String getBaseLocation() {
}

public static class Builder extends PolarisEntity.BaseBuilder<IcebergTableLikeEntity, Builder> {

public Builder(
PolarisEntitySubType subType,
TableIdentifier identifier,
Map<String, String> properties,
Map<String, String> internalProperties,
String metadataLocation) {
super();
setType(PolarisEntityType.TABLE_LIKE);
setSubType(subType);
setProperties(properties);
setInternalProperties(internalProperties);
// order here matters. properties and internal properties must be set prior to the following
// properties, which merely update the map, whereas the above calls replace the map entirely.
setTableIdentifier(identifier);
setMetadataLocation(metadataLocation);
}

public Builder(
PolarisEntitySubType subType, TableIdentifier identifier, String metadataLocation) {
super();
Expand Down Expand Up @@ -121,6 +143,29 @@ public Builder setBaseLocation(String location) {
return this;
}

@Override
public Builder setInternalProperties(@Nonnull Map<String, String> internalProperties) {
// ensure we carry forward the parent namespace and metadata location if already set.
// however, we allow for overriding them if explicitly specified in the provided map.
Map<String, String> newInternalProperties = new HashMap<>();
if (this.internalProperties.get(NamespaceEntity.PARENT_NAMESPACE_KEY) != null) {
newInternalProperties.put(
NamespaceEntity.PARENT_NAMESPACE_KEY,
this.internalProperties.get(NamespaceEntity.PARENT_NAMESPACE_KEY));
}
if (this.internalProperties.get(METADATA_LOCATION_KEY) != null) {
newInternalProperties.put(
METADATA_LOCATION_KEY, this.internalProperties.get(METADATA_LOCATION_KEY));
}
if (this.internalProperties.get(LAST_ADMITTED_NOTIFICATION_TIMESTAMP_KEY) != null) {
newInternalProperties.put(
LAST_ADMITTED_NOTIFICATION_TIMESTAMP_KEY,
this.internalProperties.get(LAST_ADMITTED_NOTIFICATION_TIMESTAMP_KEY));
}
newInternalProperties.putAll(internalProperties);
return super.setInternalProperties(newInternalProperties);
}

public Builder setMetadataLocation(String location) {
internalProperties.put(METADATA_LOCATION_KEY, location);
return this;
Expand Down
2 changes: 1 addition & 1 deletion polaris-core/src/test/java/org/apache/polaris/core/persistence/cache/EntityWeigherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void testExactWeightCalculation() {
"location",
"{\"a\": \"b\"}",
Optional.of("{\"c\": \"d\", \"e\": \"f\"}")));
Assertions.assertThat(preciseWeight).isEqualTo(1090);
Assertions.assertThat(preciseWeight).isEqualTo(1255); // :( this is hard-coded
}

private static Map<String, String> getPropertiesMap(String properties) {
Expand Down
33 changes: 32 additions & 1 deletion runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1566,14 +1566,44 @@ public void doCommit(TableMetadata base, TableMetadata metadata) {
"Generic table with same name already exists: %s", tableIdentifier);
}
}
Map<String, String> storedProperties = new HashMap<>();
Comment thread
collado-mike marked this conversation as resolved.
Outdated
storedProperties.put("location", metadata.location());
Comment thread
collado-mike marked this conversation as resolved.
Outdated
storedProperties.put("format-version", String.valueOf(metadata.formatVersion()));
storedProperties.put("table-uuid", metadata.uuid());
storedProperties.put("current-schema-id", String.valueOf(metadata.currentSchemaId()));
if (metadata.currentSnapshot() != null) {
storedProperties.put(
"current-snapshot-id", String.valueOf(metadata.currentSnapshot().snapshotId()));
}
storedProperties.put("last-column-id", String.valueOf(metadata.lastColumnId()));
storedProperties.put("next-row-id", String.valueOf(metadata.nextRowId()));
storedProperties.put("last-sequence-number", String.valueOf(metadata.lastSequenceNumber()));
storedProperties.put("last-updated-ms", String.valueOf(metadata.lastUpdatedMillis()));
if (metadata.sortOrder() != null) {
storedProperties.put(
"default-sort-order-id", String.valueOf(metadata.defaultSortOrderId()));
}
if (metadata.spec() != null) {
storedProperties.put("default-spec-id", String.valueOf(metadata.defaultSpecId()));
storedProperties.put(
"last-partition-id", String.valueOf(metadata.lastAssignedPartitionId()));
}
if (metadata.currentSnapshot() != null) {
storedProperties.put(
"current-snapshot-id", String.valueOf(metadata.currentSnapshot().snapshotId()));
}
IcebergTableLikeEntity entity =
IcebergTableLikeEntity.of(resolvedPath == null ? null : resolvedPath.getRawLeafEntity());
String existingLocation;
if (null == entity) {
existingLocation = null;
entity =
new IcebergTableLikeEntity.Builder(
PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier, newLocation)
PolarisEntitySubType.ICEBERG_TABLE,
tableIdentifier,
Map.of(),

@dimas-b dimas-b Oct 2, 2025

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.

nit: If we're using the builder pattern, why have rich constructor parameters? Why not call .setABC()?

In this case the Map is empty, so this parameter is redundant?

@collado-mike collado-mike Oct 3, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem is in setting the internalProperties map in the builder methods because we have these helper methods, such as setMetadataLocation that aren't fields themselves, but modify entries in the map. If we do call the following, we're fine

builder
    .setInternalProperties(props)
    .setMetadataLocation(newLocation)
    .build();

but if we reverse the order and call the following, we're broken:

builder
    .setMetadataLocation(newLocation)
    .setInternalProperties(props)
    .build();

It's not obvious from the caller's perspective, but setMetadataLocation modifies the underlying map, then the setInternalProperties call completely overwrites the map, losing the value set in the previous call.

With the existing constructor, it's impossible to order the setting of the metadataLocation and the internalProperties via the builder methods. If we used the builder for all properties all the time, it would be fine, but because we pass in the newLocation parameter as a constructor arg, if we set the internalProperties field using the builder method, we lose the location parameter we just passed in.

@dimas-b dimas-b Oct 3, 2025 •
edited
Loading

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.

Good point, TIL 🤔 However, having this kind of effects in the codebase it pretty risky in the long term maintenance perspective, IMHO. Would it be reasonable to refactor the builders / related code to allow for more intuitive usage (in another PR, of course)?

@collado-mike collado-mike Oct 3, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about making metadataLocation and other settable map entries into distinct fields in the Builder so that they can just be added to the map in the build call, but... yeah, a future PR

storedProperties,
newLocation)
.setCatalogId(getCatalogId())
.setBaseLocation(metadata.location())
.setId(
Expand All @@ -1583,6 +1613,7 @@ public void doCommit(TableMetadata base, TableMetadata metadata) {
existingLocation = entity.getMetadataLocation();
entity =
new IcebergTableLikeEntity.Builder(entity)
.setInternalProperties(storedProperties)
.setBaseLocation(metadata.location())
.setMetadataLocation(newLocation)
.build();
Expand Down
88 changes: 88 additions & 0 deletions .../src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import java.lang.reflect.Method;
import java.time.Clock;
import java.util.Arrays;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -62,6 +63,7 @@
import org.apache.iceberg.FileMetadata;
import org.apache.iceberg.FileScanTask;
import org.apache.iceberg.MetadataUpdate;
import org.apache.iceberg.NullOrder;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.RowDelta;
import org.apache.iceberg.Schema;
Expand Down Expand Up @@ -103,6 +105,7 @@
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.entity.CatalogEntity;
import org.apache.polaris.core.entity.NamespaceEntity;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntitySubType;
Expand Down Expand Up @@ -154,6 +157,7 @@
import org.apache.polaris.service.types.TableUpdateNotification;
import org.assertj.core.api.AbstractCollectionAssert;
import org.assertj.core.api.Assertions;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.assertj.core.api.ListAssert;
import org.assertj.core.api.ThrowableAssert.ThrowingCallable;
import org.assertj.core.configuration.PreferredAssumptionException;
Expand Down Expand Up @@ -2282,6 +2286,90 @@ public void testTableOperationsDoesNotRefreshAfterCommit(boolean updateMetadataO
}
}

@Test
public void testTableInternalPropertiesStoredOnCommit() {

@HonahX HonahX Oct 4, 2025

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.

[Non-blocker] Since we are testing the extraction of metadata fields, it would be good if we could add/or parametrize this test with different format-version (1,2,3). Technically the TableMetadata should be format-version agnostic, but a sanity check would be useful in case there's some upstream bug : )

Assumptions.assumeTrue(
requiresNamespaceCreate(),
"Only applicable if namespaces must be created before adding children");

catalog.createNamespace(NS);
catalog.buildTable(TABLE, SCHEMA).create();
catalog.loadTable(TABLE).newFastAppend().appendFile(FILE_A).commit();
Table afterAppend = catalog.loadTable(TABLE);
EntityResult schemaResult =

@HonahX HonahX Oct 4, 2025

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.

Suggested change
EntityResult schemaResult =
EntityResult namespaceResult =

[nit] Let's use namespace here to be consistent with polaris' terminology

metaStoreManager.readEntityByName(
polarisContext,
List.of(catalogEntity),
PolarisEntityType.NAMESPACE,
PolarisEntitySubType.NULL_SUBTYPE,
NS.toString());
Assertions.assertThat(schemaResult).returns(true, EntityResult::isSuccess);
EntityResult tableResult =
metaStoreManager.readEntityByName(
polarisContext,
List.of(catalogEntity, schemaResult.getEntity()),
PolarisEntityType.TABLE_LIKE,
PolarisEntitySubType.ICEBERG_TABLE,
TABLE.name());
Assertions.assertThat(tableResult)
.returns(true, EntityResult::isSuccess)
.extracting(er -> PolarisEntity.of(er.getEntity()))
.extracting(PolarisEntity::getInternalPropertiesAsMap)
.asInstanceOf(InstanceOfAssertFactories.map(String.class, String.class))
.containsEntry(NamespaceEntity.PARENT_NAMESPACE_KEY, NS.toString())
.containsEntry(
"current-snapshot-id", String.valueOf(afterAppend.currentSnapshot().snapshotId()))
.containsEntry("location", afterAppend.location())
.containsEntry("table-uuid", afterAppend.uuid().toString())
.containsEntry("current-schema-id", String.valueOf(afterAppend.schema().schemaId()))
.containsEntry(
"last-column-id",
afterAppend.schema().columns().stream()
.max(Comparator.comparing(Types.NestedField::fieldId))
.map(Types.NestedField::fieldId)
.orElse(0)
.toString())
.containsEntry(
"last-sequence-number", String.valueOf(afterAppend.currentSnapshot().sequenceNumber()));

catalog.loadTable(TABLE).refresh();
catalog.loadTable(TABLE).newFastAppend().appendFile(FILE_B).commit();
validatePropertiesUpdated(
schemaResult,
"current-snapshot-id",
tbl -> String.valueOf(tbl.currentSnapshot().snapshotId()));

catalog.loadTable(TABLE).refresh();
catalog.loadTable(TABLE).updateSchema().addColumn("new_col", Types.LongType.get()).commit();
validatePropertiesUpdated(
schemaResult, "current-schema-id", tbl -> String.valueOf(tbl.schema().schemaId()));

catalog.loadTable(TABLE).refresh();
catalog.loadTable(TABLE).replaceSortOrder().desc("new_col", NullOrder.NULLS_FIRST).commit();
validatePropertiesUpdated(
schemaResult,
"default-sort-order-id",
table -> String.valueOf(table.sortOrder().orderId()));
}

private void validatePropertiesUpdated(
EntityResult schemaResult, String key, Function<Table, String> expectedValue) {
Table afterUpdate = catalog.loadTable(TABLE);
EntityResult tableResult =
metaStoreManager.readEntityByName(
polarisContext,
List.of(catalogEntity, schemaResult.getEntity()),
PolarisEntityType.TABLE_LIKE,
PolarisEntitySubType.ICEBERG_TABLE,
TABLE.name());
Assertions.assertThat(tableResult)
.returns(true, EntityResult::isSuccess)
.extracting(er -> PolarisEntity.of(er.getEntity()))
.extracting(PolarisEntity::getInternalPropertiesAsMap)
.asInstanceOf(InstanceOfAssertFactories.map(String.class, String.class))
.containsEntry(key, expectedValue.apply(afterUpdate));
}

@Test
public void testEventsAreEmitted() {
IcebergCatalog catalog = catalog();
Expand Down
Loading