Skip to content

Remove unused private constructors from core #3200

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

Closed
dimas-b wants to merge 1 commit into from
Closed

Remove unused private constructors from core #3200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 0 additions & 35 deletions 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 @@ -18,9 +18,7 @@
*/
package org.apache.polaris.core.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.HashMap;
Expand Down Expand Up @@ -87,39 +85,6 @@ public String getName() {
}
}

@JsonCreator
private PolarisEntity(
@JsonProperty("catalogId") long catalogId,
@JsonProperty("typeCode") PolarisEntityType type,
@JsonProperty("subTypeCode") PolarisEntitySubType subType,
@JsonProperty("id") long id,
@JsonProperty("parentId") long parentId,
@JsonProperty("name") String name,
@JsonProperty("createTimestamp") long createTimestamp,
@JsonProperty("dropTimestamp") long dropTimestamp,
@JsonProperty("purgeTimestamp") long purgeTimestamp,
@JsonProperty("lastUpdateTimestamp") long lastUpdateTimestamp,
@JsonProperty("properties") String properties,
@JsonProperty("internalProperties") String internalProperties,
@JsonProperty("entityVersion") int entityVersion,
@JsonProperty("grantRecordsVersion") int grantRecordsVersion) {
super(
catalogId,
id,
type.getCode(),
parentId,
name,
subType.getCode(),
createTimestamp,
dropTimestamp,
purgeTimestamp,
lastUpdateTimestamp,
properties,
internalProperties,
grantRecordsVersion,
entityVersion);
}

public PolarisEntity(
long catalogId,
PolarisEntityType type,
Expand Down
12 changes: 0 additions & 12 deletions ...re/src/main/java/org/apache/polaris/core/persistence/dao/entity/ChangeTrackingResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.List;
Expand Down Expand Up @@ -52,16 +50,6 @@ public ChangeTrackingResult(@Nonnull List<PolarisChangeTrackingVersions> changeT
this.changeTrackingVersions = changeTrackingVersions;
}

@JsonCreator
private ChangeTrackingResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") String extraInformation,
@JsonProperty("changeTrackingVersions")
List<PolarisChangeTrackingVersions> changeTrackingVersions) {
super(returnStatus, extraInformation);
this.changeTrackingVersions = changeTrackingVersions;
}

public List<PolarisChangeTrackingVersions> getChangeTrackingVersions() {
return changeTrackingVersions;
}
Expand Down
13 changes: 0 additions & 13 deletions ...ore/src/main/java/org/apache/polaris/core/persistence/dao/entity/CreateCatalogResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.entity.PolarisBaseEntity;
Expand Down Expand Up @@ -58,17 +56,6 @@ public CreateCatalogResult(
this.catalogAdminRole = catalogAdminRole;
}

@JsonCreator
private CreateCatalogResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") @Nullable String extraInformation,
@JsonProperty("catalog") @Nonnull PolarisBaseEntity catalog,
@JsonProperty("catalogAdminRole") @Nonnull PolarisBaseEntity catalogAdminRole) {
super(returnStatus, extraInformation);
this.catalog = catalog;
this.catalogAdminRole = catalogAdminRole;
}

public PolarisBaseEntity getCatalog() {
return catalog;
}
Expand Down
13 changes: 0 additions & 13 deletions ...e/src/main/java/org/apache/polaris/core/persistence/dao/entity/CreatePrincipalResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.entity.PolarisBaseEntity;
Expand Down Expand Up @@ -59,17 +57,6 @@ public CreatePrincipalResult(
this.principalSecrets = principalSecrets;
}

@JsonCreator
private CreatePrincipalResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") @Nullable String extraInformation,
@JsonProperty("principal") @Nonnull PolarisBaseEntity principal,
@JsonProperty("principalSecrets") @Nonnull PolarisPrincipalSecrets principalSecrets) {
super(returnStatus, extraInformation);
this.principal = principal;
this.principalSecrets = principalSecrets;
}

public PrincipalEntity getPrincipal() {
return PrincipalEntity.of(principal);
}
Expand Down
11 changes: 0 additions & 11 deletions ...s-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/DropEntityResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;

Expand Down Expand Up @@ -57,15 +55,6 @@ public DropEntityResult(long cleanupTaskId) {
this.cleanupTaskId = cleanupTaskId;
}

@JsonCreator
private DropEntityResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") String extraInformation,
@JsonProperty("cleanupTaskId") Long cleanupTaskId) {
super(returnStatus, extraInformation);
this.cleanupTaskId = cleanupTaskId;
}

public Long getCleanupTaskId() {
return cleanupTaskId;
}
Expand Down
11 changes: 0 additions & 11 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/EntityResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.entity.PolarisBaseEntity;
Expand Down Expand Up @@ -87,15 +85,6 @@ public PolarisEntitySubType getAlreadyExistsEntitySubType() {
}
}

@JsonCreator
private EntityResult(
Copy link
Contributor

@HonahX HonahX Dec 4, 2025

Choose a reason for hiding this comment

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

Hi @dimas-b Thanks for the effort of improving clarity of the core module. These private constructors are there for these classes to be JSON Serdes, which will be useful for downstream integration where the MetaStoreManager is talking to a remote persistence server via Rest API.

Could we keep these constructor to preserve the JSON Serdes for these classes. It would also be ok if we replace these with static JsonCreator if that help make things clearer.

I am also thinking of adding roundtrip test for these classes.

Copy link
Contributor Author

@dimas-b dimas-b Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the info, @HonahX !

However, from the OSS perspective this use case is absolutely not apparent 🤷 Adding tests can be a means for avoiding regressions in downstream, but the situation is still that a downstream project imposes a restriction on Polaris code in this case without any OSS use cases.

@flyrain : What is your take on this in light of the recent dev ML conversation?

I propose to use this PR as an illustration and continue the discussion of dev.

Copy link
Contributor

@HonahX HonahX Dec 5, 2025

Choose a reason for hiding this comment

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

Hi @dimas-b Thanks for sharing the thoughts!

IMHO this is not a requirement imposed by any downstream project. The JSON serde behavior has been in Polaris since the very beginning and has effectively become an existing convention in the core module. We can still see many similar annotations across the codebase today:

  • PolarisEntityId
  • PolarisEntitySubType
  • ...
    In that sense, removing these private @JsonCreator constructors is not just a cleanup — it changes the long-standing implication that these classes are expected to be JSON-serializable. Hence, I feel the situation is different from the dev ML conversion linked above

Regarding potential use cases: the downstream integration example I mentioned was just one illustration. Other scenarios, such as migrating or syncing principals or entities between Polaris deployments, could also reasonably rely on these classes being JSON serde.

In general, I would prefer that we not lose this established assumption and flexibility purely for the sake of having less code. If the private constructors themselves can be considered confusing, we could replace them with static json creator, and switching to builder/Immutable for result classes for example.

Please let me know WDYT!

Copy link
Contributor Author

@dimas-b dimas-b Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@HonahX : How can the serde removed in this PR be a convention in Polaris, when it is not used in Polaris build or runtime?

My exact point is that it is a convention in a downstream project, but not in Polaris itself.

As I mentioned above, I do not insist on merging this PR. However, it is an illustration of downstream dependencies that exists only in downstream code. Successful CI on this PR shows that Polaris itself does not rely on this code.

Ideally, IMHO, downstream projects that rely on serializing these objects should probably implement and maintain serde (via JSON or overwise) by themselves.

Copy link
Contributor

@HonahX HonahX Dec 5, 2025

Choose a reason for hiding this comment

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

Hi @dimas-b. The way I see it, even though the JSON-serde paths aren’t exercised in Polaris today, the @JsonCreator constructors have been there for long time acting as an implicit signal that these classes are intended to round-trip through JSON. In other words, the annotations themselves set an expectation for how future changes to these classes should behave, regardless of whether the current runtime calls into that path.

Whether we actually want to keep JSON serde as part of the core module’s design in the future could be a separate discussion. It seems we may have different views on that, which is totally fine — I think it’s something worth discussing with the community so we can align on the long-term direction together. I was about to raise a PR to restore JSON serde for PolarisEntityCore (which was removed as a side effect of #1596), but that should depend on the direction we decide collectively.

@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") @Nullable String extraInformation,
@JsonProperty("entity") @Nullable PolarisBaseEntity entity) {
super(returnStatus, extraInformation);
this.entity = entity;
}

public PolarisBaseEntity getEntity() {
return entity;
}
Expand Down
11 changes: 0 additions & 11 deletions .../src/main/java/org/apache/polaris/core/persistence/dao/entity/GenerateEntityIdResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;

Expand Down Expand Up @@ -51,15 +49,6 @@ public GenerateEntityIdResult(@Nonnull Long id) {
this.id = id;
}

@JsonCreator
private GenerateEntityIdResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") @Nullable String extraInformation,
@JsonProperty("id") @Nullable Long id) {
super(returnStatus, extraInformation);
this.id = id;
}

public Long getId() {
return id;
}
Expand Down
16 changes: 0 additions & 16 deletions ...s-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/LoadGrantsResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.List;
Expand Down Expand Up @@ -69,20 +67,6 @@ public LoadGrantsResult(
this.entities = entities;
}

@JsonCreator
private LoadGrantsResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") String extraInformation,
@JsonProperty("grantsVersion") int grantsVersion,
@JsonProperty("grantRecords") List<PolarisGrantRecord> grantRecords,
@JsonProperty("entities") List<PolarisBaseEntity> entities) {
super(returnStatus, extraInformation);
this.grantsVersion = grantsVersion;
this.grantRecords = grantRecords;
// old GS code might not serialize this argument
this.entities = entities;
}

public int getGrantsVersion() {
return grantsVersion;
}
Expand Down
13 changes: 0 additions & 13 deletions ...rc/main/java/org/apache/polaris/core/persistence/dao/entity/LoadPolicyMappingsResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.List;
Expand Down Expand Up @@ -64,17 +62,6 @@ public LoadPolicyMappingsResult(
this.entities = entities;
}

@JsonCreator
private LoadPolicyMappingsResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") String extraInformation,
@JsonProperty("policyMappingRecords") List<PolarisPolicyMappingRecord> mappingRecords,
@JsonProperty("policyEntities") List<PolarisBaseEntity> entities) {
super(returnStatus, extraInformation);
this.mappingRecords = mappingRecords;
this.entities = entities;
}

public List<PolarisPolicyMappingRecord> getPolicyMappingRecords() {
return mappingRecords;
}
Expand Down
11 changes: 0 additions & 11 deletions .../src/main/java/org/apache/polaris/core/persistence/dao/entity/PolicyAttachmentResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.policy.PolarisPolicyMappingRecord;
Expand Down Expand Up @@ -62,15 +60,6 @@ public PolicyAttachmentResult(@Nonnull PolarisPolicyMappingRecord mappingRecord)
this.mappingRecord = mappingRecord;
}

@JsonCreator
private PolicyAttachmentResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") String extraInformation,
@JsonProperty("policyMappingRecord") PolarisPolicyMappingRecord mappingRecord) {
super(returnStatus, extraInformation);
this.mappingRecord = mappingRecord;
}

public PolarisPolicyMappingRecord getPolicyMappingRecord() {
return mappingRecord;
}
Expand Down
11 changes: 0 additions & 11 deletions .../src/main/java/org/apache/polaris/core/persistence/dao/entity/PrincipalSecretsResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.entity.PolarisPrincipalSecrets;
Expand Down Expand Up @@ -52,15 +50,6 @@ public PrincipalSecretsResult(@Nonnull PolarisPrincipalSecrets principalSecrets)
this.principalSecrets = principalSecrets;
}

@JsonCreator
private PrincipalSecretsResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") @Nullable String extraInformation,
@JsonProperty("principalSecrets") @Nonnull PolarisPrincipalSecrets principalSecrets) {
super(returnStatus, extraInformation);
this.principalSecrets = principalSecrets;
}

public PolarisPrincipalSecrets getPrincipalSecrets() {
return principalSecrets;
}
Expand Down
11 changes: 0 additions & 11 deletions ...is-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/PrivilegeResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.entity.PolarisGrantRecord;
Expand Down Expand Up @@ -51,15 +49,6 @@ public PrivilegeResult(@Nonnull PolarisGrantRecord grantRecord) {
this.grantRecord = grantRecord;
}

@JsonCreator
private PrivilegeResult(
@JsonProperty("returnStatus") @Nonnull ReturnStatus returnStatus,
@JsonProperty("extraInformation") String extraInformation,
@JsonProperty("grantRecord") PolarisGrantRecord grantRecord) {
super(returnStatus, extraInformation);
this.grantRecord = grantRecord;
}

public PolarisGrantRecord getGrantRecord() {
return grantRecord;
}
Expand Down
11 changes: 0 additions & 11 deletions .../src/main/java/org/apache/polaris/core/persistence/dao/entity/ResolvedEntitiesResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@

package org.apache.polaris.core.persistence.dao.entity;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.List;
Expand All @@ -41,15 +39,6 @@ public ResolvedEntitiesResult(
this.resolvedEntities = null;
}

@JsonCreator
private ResolvedEntitiesResult(
@JsonProperty("returnStatus") ReturnStatus returnStatus,
@JsonProperty("extraInformation") String extraInformation,
@JsonProperty("resolvedEntities") List<ResolvedPolarisEntity> resolvedEntities) {
super(returnStatus, extraInformation);
this.resolvedEntities = resolvedEntities;
}

public List<ResolvedPolarisEntity> getResolvedEntities() {
return resolvedEntities;
}
Expand Down