Add policies for metadata compaction, orphan file removal and snapshot retention

[flyrain]

No description provided.

[leangjonathan]

I think this should have a warning that if you specify locations other than the table base location for example s3://my-bucket instead of s3://my-bucket/my-table-location, all files not referenced by the table will purged including potentially other table files if those files are stored in the specified path. I think this aligns with best practice notes that one shouldn't store tables under the same location

[flyrain]

Added a waning message.

[gurukaraje]

It might be easier to specify the file age for orphaned files in days instead of timestamp? I.e.
"max_orphan_file_age_in_days" : 30
instead of
"older_than": 1707315296

[leangjonathan]

+1. Timestamp probably only makes sense for engines during operation runtime

[flyrain]

Good point, made the change.

[gurukaraje]

how about keeping all the 3 policies consistent with the format? metadata compaction and snapshot-expiry seem to follow the format:

"enable": true
"config" : {
   "key1" : value1
   "key2" : value2
}

But orphan file deletion has some of the properties older_than and location out side of config:

"enable": true,
"older_than": 1707315296,
"location": "s3://my-bucket/my-table-location",
"config": {
   "prefix_mismatch_mode": "ignore",
   "my_key": "my_value"
}

[flyrain]

The config map is used for customized properties. For example, a TMS may provide an optional feature that only a subset of tables need it.

[flyrain]

This hard to read in an IDE like IntelliJ as it is a single long line. Json doesn't support a way to break one line to multiple lines. This makes me think we may use the format yaml instead of json.

[eric-maynard]

The alternative is just to put line breaks in the string, and then preprocess it anywhere we want to strip out the whitespace

[flyrain]

put line breaks in the string

One of my versions was that :). It didn't help much, esp. in the IDE.

[ashvina]

What do you think about making this property multi-value? This way, a user could support adding paths for multiple "namespaces".

[eric-maynard]

I guess you don't mean namespaces but just "locations"?

I think having one policy map to one location is probably okay for now; I'm not sure if/how we plan to handle overlapping locations though.

[flyrain]

We could support that. One use case is that table files might be stored in different locations based on the write.data.path and/or write.metadata.path settings. This is generally not recommended though, due to issues like it makes credential vending harder. Are there any other use cases you have in mind, @ashvina?

[flyrain]

Overlapping locations is very dangerous. No orphan file removal should happen in that case.

[flyrain]

Made it a string array type in the new commit.

[eric-maynard]

1. What does Inheritable mean here?
2. Polaris seems redundant, all of these are going to be Polaris schemas right?

[flyrain]

A inheritable policy means it can be applied to the under layer entities. For example, all tables under a namespace get the policies if it is assigned to the namespace.
I'm OK to remove it or keep it. Keeping it provides a complete view for anyone who read the schema, but without too much context of Polaris.

[eric-maynard]

Are all policies inheritable?

[flyrain]

No. For example, column masking policy are not inheritable, which makes more sense, and also that's what most engines do.

[eric-maynard]

Does the JSON magically land at the location specified in the ID somehow? Or do we always need a followup PR?

Also, it looks a little funny to use dates here given that the date in the PR may not align with the date the schema actually becomes effective. In the worst case, we could merge two versions in one day. Maybe just an incrementing number is easier?

[flyrain]

Unfortunately no. I hope I can publish it once for all based on the directory structure.

I want to keep them as the same for date as these are first batch, should be fine as nobody is using it. Once we release them at 1.0. We should follow the date schema strictly.

[snazy]

Adding a note here as we might need a couple more days to review this one. Please do not merge yet.

[flyrain]

Adding a note here as we might need a couple more days to review this one. Please do not merge yet.

@snazy, this PR was filed 10 days ago, and it's a small one. Can you review it in a timely manner?

[snazy]

My concern with this change, and it's quickly merged predecessor #945, is that AFAIK we don't have a consensus on policies in general. And specifically where these files should live, how those are accessed by users and how those are used by implementations.

I don't object on draft-code and draft-PRs, but I think that we should have a consensus first on those rather big topics before merging anything. Sure, this change is rather "small" or "innocent", but the topic is big.

[flyrain]

I believe we have reached a general consensus on policy management:

1. Design Document: The design document [1] was published over two months ago, triggering a lot of discussions. Stakeholders from various companies—including representatives from Dremio—actively contributed, and all feedback has been incorporated without any outstanding blockers.
2. Review Sessions: We conducted multiple review sessions with key stakeholders such as @jbonofre, @omarsmak, @RussellSpitzer and several others. Through these discussions, we aligned on the overall approach.
3. PR Add data compaction policy schema #945 was received strong support, not only from committer @eric-maynard but also from @omarsmak, one of the stakeholders from Dremio and @HonahX, confirming broad agreement.

[1] https://docs.google.com/document/d/1kIiVkFFg9tPa5SH70b9WwzbmclrzH3qWHKfCKXw5lbs/edit?tab=t.0

[omarsmak]

@flyrain this also struggles me a bit, remove orphan policy can be even expressed in more than in just file age. We don't we opt for the config key similar to the other policies?

[flyrain]

remove orphan policy can be even expressed in more than in just file age.

Can you name them? We can put them into schema if they are commonly used, otherwise, the config map would be the best place to be.

[omarsmak]

I'd vote for the config map

[flyrain]

If no extra field is suggested, we could keep it as is.

[snazy]

I believe we have reached a general consensus on policy management:

1. Design Document: The design document [1] was published over two months ago, triggering a lot of discussions. Stakeholders from various companies—including representatives from Dremio—actively contributed, and all feedback has been incorporated without any outstanding blockers.

2. Review Sessions: We conducted multiple review sessions with key stakeholders such as @jbonofre, @omarsmak, @RussellSpitzer and several others. Through these discussions, we aligned on the overall approach.

3. PR [Add data compaction policy schema #945](https://github.com/apache/polaris/pull/945) was received strong support, not only from committer @eric-maynard but also from @omarsmak, one of the stakeholders from Dremio and @HonahX, confirming broad agreement.

[1] https://docs.google.com/document/d/1Vuhw5b9-6KAol2vU3HUs9FJwcgWtiVVXMYhLtGmz53s/edit?tab=t.0

(Not sure how the linked google doc is related to this PR)

The points I raised are about:

1. The changes committed to the production code base are not used, there is just no code that uses those - and won't be used soon. Those belong to a topic that's overall still WIP - hence I object merging it into main at this point.
2. These files land in a place that makes it extremely hard for arbitrary consumers to consume those. I do not think that those should live there.
3. Add data compaction policy schema #945 was merged without giving all contributors enough time to review.

I propose to move the work to a feature branch and go from there.

[flyrain]

Update the doc link.

The changes committed to the production code base are not used, there is just no code that uses those - and won't be used soon. Those belong to a topic that's overall still WIP - hence I object merging it into main at this point.

It's a common and recommended way to break a big feature into multiple small PRs, which not only makes the process iterating faster, but also improve review/code quality. A separate branch is only necessary when a feature introduces breaking changes, which is not the case for this PR.

These files land in a place that makes it extremely hard for arbitrary consumers to consume those. I do not think that those should live there.

I'm surprised by this claim. It's a common practice to put non-code files in the dir resources, it's pretty easy for any Java code to consume. For example, #938 introduced a way to consume these schemas. We also plan to publish them in the website for other tools to consume.