Add data compaction policy validator #1238

flyrain · 2025-03-21T17:21:23Z

This PR adds interface PolicyValidator and one implementation for data compaction policy. Policy validator is used at policy creation/update as well as attaching a policy to a Polaris entity.
For more context, check
the design doc: https://docs.google.com/document/d/1kIiVkFFg9tPa5SH70b9WwzbmclrzH3qWHKfCKXw5lbs/edit?tab=t.0#heading=h.nly223xz13km
cc @HonahX

flyrain · 2025-03-21T18:39:04Z

The failure seems unrelated.

QuarkusApplicationIntegrationTest > initializationError FAILED
    java.lang.NullPointerException at PolarisApplicationIntegrationTest.java:137
Timed out trying to set fork join ClassLoader, this should never happen unless something has tied up a fork join thread before the app launched [Error Occurred After Shutdown]

OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
Timed out trying to set fork join ClassLoader, this should never happen unless something has tied up a fork join thread before the app launched [Error Occurred After Shutdown]

> Task :polaris-quarkus-service:test

QuarkusRestCatalogIntegrationTest > initializationError FAILED
    java.lang.NullPointerException at PolarisRestCatalogIntegrationTest.java:173
Timed out trying to set fork join ClassLoader, this should never happen unless something has tied up a fork join thread before the app launched [Error Occurred After Shutdown]


> Task :polaris-quarkus-service:test

QuarkusRestCatalogViewAzureIntegrationTest > initializationError FAILED
    java.lang.NullPointerException at PolarisRestCatalogViewIntegrationBase.java:76
Timed out trying to set fork join ClassLoader, this should never happen unless something has tied up a fork join thread before the app launched [Error Occurred After Shutdown]

HonahX

Thanks @flyrain ! This looks like a great start of Policy Validators. Overall LGTM!

HonahX · 2025-03-21T23:49:25Z

polaris-core/src/main/java/org/apache/polaris/core/policy/validator/GenericPolicyValidator.java

+      case METADATA_COMPACTION:
+      case SNAPSHOT_RETENTION:
+      case ORPHAN_FILE_REMOVAL:
+      default:


Just check my understanding: in the future when we add support for custom type, we will need to load the custom type's validator here?

yeah, something like this:

ctor = DynConstructors.builder(PolicyValidator.class).impl(impl).buildChecked(); policyValidator = ctor.newInstance();

HonahX · 2025-03-21T23:51:38Z

polaris-core/src/main/java/org/apache/polaris/core/policy/validator/GenericPolicyValidator.java

+   * @param targetEntity the target Polaris entity to attach the policy to
+   * @return {@code true} if the policy is attachable to the target entity; {@code false} otherwise
+   */
+  public static boolean canAttach(PolicyEntity policy, PolarisEntity targetEntity) {


Just brainstorming about the name, may be "isAttachable"?

The method’s primary role is to decide whether a policy is capable of being attached to a target, "canAttach" can be appropriate. I think isAttachable is more suitable if the method only takes a policy entity. WDYT?

HonahX · 2025-03-21T23:53:32Z

polaris-core/src/main/java/org/apache/polaris/core/policy/validator/InvalidPolicyException.java

+import org.apache.polaris.core.exceptions.PolarisException;
+
+/** Exception thrown when a policy is invalid or violates defined rules. */
+public class InvalidPolicyException extends PolarisException {


Should this map to status code 400, BadRequestError in rest?

HonahX · 2025-03-22T00:05:15Z

...re/src/main/java/org/apache/polaris/core/policy/validator/DataCompactionPolicyValidator.java

+    }
+
+    try {
+      var policy = PolicyValidatorUtil.MAPPER.readValue(content, DataCompactionPolicy.class);


Suggested change

var policy = PolicyValidatorUtil.MAPPER.readValue(content, DataCompactionPolicy.class);

DataCompactionPolicy policy = PolicyValidatorUtil.MAPPER.readValue(content, DataCompactionPolicy.class);

Shall we explicitly write out the type here?

I'm OK with either way.

HonahX · 2025-03-22T00:10:13Z

...re/src/main/java/org/apache/polaris/core/policy/validator/DataCompactionPolicyValidator.java

+      if (!POLICY_SCHEMA_VERSIONS.contains(policy.getVersion())) {
+        throw new InvalidPolicyException("Invalid policy version: " + policy.getVersion());
+      }
+


This may be a follow up: Do we need to validate data compaction configs (if they present). For example, target_file_size_bytes needs to be a value larger than 0.

We cannot know the semantic within the config map. User can give a string to target_file_size_bytes, it should still be acceptable, as target_file_size_bytes doesn't mean anything in the schema.

HonahX · 2025-03-22T00:12:48Z

polaris-core/src/main/java/org/apache/polaris/core/policy/validator/DataCompactionPolicy.java

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.policy.validator;


Shall we move Data Compaction Policy Validator to a submodule data-compaction? I think a submodule for each policy type can offer a more organized layout.

you mean a sub package? make sense, will move it.

HonahX · 2025-03-22T00:14:34Z

polaris-core/src/main/java/org/apache/polaris/core/policy/validator/DataCompactionPolicy.java

+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import java.util.Map;
+
+public class DataCompactionPolicy {


Suggested change

public class DataCompactionPolicy {

public class DataCompactionPolicyContent {

Shall we append "Content" to the name to differentiate it with the Policy class generated by open api generator?

Yufei Gu added 3 commits March 19, 2025 21:31

Add policy validator

349fe9f

Add policy validator

0b6929d

Add policy validator

4922476

flyrain marked this pull request as ready for review March 21, 2025 17:26

flyrain requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode and ebyhr as code owners March 21, 2025 17:26

Yufei Gu added 2 commits March 21, 2025 13:20

add Java doc

d629ba9

style fix

29d14ad

HonahX reviewed Mar 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data compaction policy validator #1238

Add data compaction policy validator #1238

flyrain commented Mar 21, 2025 •

edited

Loading

flyrain commented Mar 21, 2025

HonahX left a comment

HonahX Mar 21, 2025

flyrain Mar 22, 2025

HonahX Mar 21, 2025

flyrain Mar 22, 2025

HonahX Mar 21, 2025

HonahX Mar 22, 2025

flyrain Mar 22, 2025

HonahX Mar 22, 2025

flyrain Mar 22, 2025

HonahX Mar 22, 2025

flyrain Mar 22, 2025 •

edited

Loading

HonahX Mar 22, 2025

	var policy = PolicyValidatorUtil.MAPPER.readValue(content, DataCompactionPolicy.class);
	DataCompactionPolicy policy = PolicyValidatorUtil.MAPPER.readValue(content, DataCompactionPolicy.class);

	public class DataCompactionPolicy {
	public class DataCompactionPolicyContent {

Add data compaction policy validator #1238

Are you sure you want to change the base?

Add data compaction policy validator #1238

Conversation

flyrain commented Mar 21, 2025 • edited Loading

flyrain commented Mar 21, 2025

HonahX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain commented Mar 21, 2025 •

edited

Loading

flyrain Mar 22, 2025 •

edited

Loading