Skip to content
Merged
48 changes: 48 additions & 0 deletions api/src/main/java/org/apache/iceberg/actions/RewriteDeletes.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.iceberg.actions;

import java.util.Set;
import org.apache.iceberg.DeleteFile;

public interface RewriteDeletes extends SnapshotUpdate<RewriteDeletes, RewriteDeletes.Result> {

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: shall we add some Javadoc similarly to RewriteDataFiles?

/**
 * An action for rewriting delete files according to a rewrite strategy.
 * Generally used for optimizing the sizing and layout of delete files within a table.
 */

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: RewriteDeletes -> RewriteDeleteFiles to match RewriteDataFiles for consistency?

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.

Will it make sense to expose a filter? I guess it will be common to apply this optimization on specific partitions. We have this option for the data rewrite action.

Something like this?

  RewriteDeleteFiles filter(Expression expression);

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.

That sounds reasonable to me. Will do.


/**
* Set the implementation class name for rewrite strategy.
*
* @return this for method chaining
*/
RewriteDeletes strategy(String strategyImpl);

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.

Is it possible for us to pass in RewriteDeleteStrategy here rather than a free formed string?

@chenjunjiedada chenjunjiedada Aug 4, 2021

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.

RewriteDeleteStrategy is defined in the core package, while this is in the api package. This is like the pattern for the Catalog, as the usage of loading catalog:

Catalog catalog = CatalogUtil.loadCatalog(HiveCatalog.class.getName(), "hive", ImmutableMap.of(), hadoopConf);

we could have a similar usage like:

actionsProvider()
  .rewriteDeletes(table)
  .strategy(ConvertEqDeletesStrategy.class.getName())
  .execute();

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.

For data file rewrites, we didn't allow custom extension because we don't think that there's a lot of value in it just yet and we want to keep things simple if there isn't a use for dynamically loaded classes. I think we should probably take the same approach here.

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.

+1 for not having a custom impl here because if you think some strategy is valuable, it's likely also valuable for other users. Or maybe could you provide more context about what customization you would like to do in addition?

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.

I think we all agree that eq->pos is the one that can share with others. Let me update this.

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.

+1 here too. We debated this while designing the new data compaction API.

@aokolnychyi aokolnychyi Sep 17, 2021

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.

I think another common strategy would be to just compact delete files. For example, take 100 position deletes and write them as one (within a partition). Another use case is compacting a large number of small global deletes.

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.

+1. That is the next action, I have verified internally that it could bring some benefit for query.


/**
* The action result that contains a summary of the execution.
*/
interface Result {
/**
* Returns the delete files to rewrite.
*/
Set<DeleteFile> deleteFilesToReplace();

@aokolnychyi aokolnychyi Sep 17, 2021

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.

Based on the name of these methods, I assume the commit will happen outside of the action? I think this is different compared to what @RussellSpitzer has in the new data compaction action.

I believe the commit logic we use during the data compaction would also apply here as we can compact individual partitions concurrently. I'd love to see the same optimizations we added for data file rewrites. They make a big difference for large tables.

@RussellSpitzer, thoughts?

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.

it is committed in the action, see the action implementation here: https://github.com/apache/iceberg/pull/2364/files#diff-0f115e8104a36761f3afa09592e7beb70dcfeca2e5b088a05ed76a5778bf6178R84.

The option() is used to extend features like that. We can implement partially committing feature in the specific strategy according to the options passed in. I have considered this before.


/**
* Returns the added delete files.
*/
Set<DeleteFile> deleteFilesToAdd();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.iceberg.actions;

import java.util.Map;
import java.util.Set;
import org.apache.iceberg.DeleteFile;
import org.apache.iceberg.FileScanTask;
import org.apache.iceberg.Table;

public interface RewriteDeleteStrategy {

@jackye1995 jackye1995 Jul 19, 2021

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.

I think we also need the methods similar to RewriteStrategy such as name, validOptions

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.

I didn't add validOptions because we haven't finalized what options are valid and it can be added late. But I'm fine to add them now.

For name, It looks like the class name already includes the strategy name. Do we still need it?

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.

name is mostly used for logging purpose. I agree it is already in the class name, but for consistency with RewriteStrategy I think we can still have it here.

/**
* Returns the name of this rewrite deletes strategy
*/
String name();

/**
* Returns the table being modified by this rewrite strategy
*/
Table table();

/**
* Returns a set of options which this rewrite strategy can use. This is an allowed-list and any options not
* specified here will be rejected at runtime.
*/
Set<String> validOptions();

/**
* Sets options to be used with this strategy
*/
RewriteDeleteStrategy options(Map<String, String> options);

/**
* Select the deletes to rewrite.
*
* @param dataFiles iterable of FileScanTasks for data files in a given partition
* @return iterable of original delete file to be replaced.
*/
Iterable<DeleteFile> selectDeletesToRewrite(Iterable<FileScanTask> dataFiles);

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.

Why accept FileScanTask here? Will the delete action only find delete files based on a scan?

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.

This was discussed in #2841 (comment). It is in order to align with RewriteStrategy.

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.

I think as long as we follow a similar pattern it's fine, but it's more important to make the interface suitable for deletes. Here we can consider 2 cases:

  1. rewrite equality -> position: we actually need the reverse of the current information, which is a list of data files associated with the delete file.
  2. rewrite position -> data: the current interface using FileScanTask seems suitable because we need to generate something similar for rewrite anyway.

I see a few different ways we can go with this:

  1. use this current interface, the "scan task" is just a holder of delete and data files organized in the form ready for a scan. equality -> position can then reorganize information.
  2. invent a new interface suitable for both use cases, something like a Iterable<RewriteDeleteTask>
  3. instead of having a single rewrite strategy for both, having one for each, so that we can have something customized for equality delete.

@aokolnychyi aokolnychyi Sep 17, 2021

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.

I think there is a third use case: simply compacting position deletes.

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.

Also, will position -> data be something we take here or in RewriteDataFiles? I can see it being part of the data compaction. Like if we detect that certain data files have too many matching delete files, we want to rewrite them.

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.

I agree. This is the major compaction that we mentioned before. And I think this may be already available through RewriteDataFiles.

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.

@jackye1995, In order to make API more generic, maybe we can go back to use empty args and let the strategy implementation maintain the map like the implementation here. So that users could choose the deletes in their own way, for example, read from manifest directly. And since we are rewriting deletes, it looks more natural to return Iterable<DeleteFile>.

@rdblue @jackye1995 WDYT?

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.

@aokolnychyi thanks for the feedback!

I think there is a third use case: simply compacting position deletes.

Yes agree, that maps to the Hive minor compaction as well.

will position -> data be something we take here or in RewriteDataFiles? I can see it being part of the data compaction. Like if we detect that certain data files have too many matching delete files, we want to rewrite them.

I remember I had a discussion around this with Russell when he was implementing bin packing. When we rewrite data files, yes position delete information is merged to that, but delete files are not dropped, because the delete file might still reference other files that are not included during bin-packing. We discussed the possibility for also adding that feature in the data compaction action, the conclusion was to have another action specifically for this use case to (1) not make data compaction too complicated, (2) have separated actions for different purposes, (3) have better SLA control for different actions.


/**
* Define how to rewrite the deletes.
*
* @param deleteFilesToRewrite a group of files to be rewritten together
* @return iterable of delete files used to replace the original delete files.
*/
Set<DeleteFile> rewriteDeletes(Set<DeleteFile> deleteFilesToRewrite);

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.

How does this strategy handle sequence numbers? Are all of the new delete files committed to the table at the next sequence number? If so, wouldn't that mean that this may only return position delete files?

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.

Good question! Besides converting to position deletes we could split equality delete into small ones so that each file scan can carry small equality deletes as Jack mentioned before. But as you pointed out, I think it would be hard to handle the sequence number since the committed snapshot should be immutable. Can we do the trick?

I'm not sure how much benefit that splitting can provide, but I believe small files bring worse problems. How about we make rewriteDeletes more specific? Like removeEqDeletes and rewritePosDeletes.

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.

When I was talking about that use case I was also in experimental phase, but so far what I see is that in practice the 2 most commonly used ones are still (1) equality -> position, (2) position -> data. Splitting equality deletes is most of the time not worth the computation effort because it's already doing a planning and it's more cost efficient to just rewrite it to position deletes.

Going back to the conversation we are having for the interface for selectDeletesToRewrite, it feels like we should have 2 different interfaces for rewriting equality and position deletes so that their interfaces can be a bit more specialized for their own use cases?

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.

Thanks for the verification! @jackye1995. Now, I'm inclined to use more specific ones. @rdblue WDYT?

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.

Since we can choose a specific rewrite strategy in RewriteDeleteFiles, and we don't have another strategy to handle equality deletes, I think we could keep this one as it is. Does that sound reasonable to you? @rdblue @jackye1995 .


/**
* Groups file scans into lists which will be processed in a single executable unit. Each group will end up being
* committed as an independent set of changes. This creates the jobs which will eventually be run as by the underlying
* Action.
*
* @param deleteFiles iterable of DeleteFile to be rewritten
* @return iterable of lists of FileScanTasks which will be processed together
*/
Iterable<Set<DeleteFile>> planDeleteGroups(Iterable<DeleteFile> deleteFiles);
}