Skip to content

API: add an action API for rewrite deletes #2841

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
aokolnychyi merged 14 commits into from
Nov 2, 2021
Merged

API: add an action API for rewrite deletes #2841

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a2eee90
API: add an action API for rewrite deletes
chenjunjiedada Jul 15, 2021
ed51fc7
add more API functions
chenjunjiedada Jul 20, 2021
0697cf5
update according to Jack's comments
chenjunjiedada Jul 30, 2021
9537fa1
use more common api
chenjunjiedada Jul 30, 2021
c68303b
address comments
chenjunjiedada Sep 15, 2021
6014276
address comments
chenjunjiedada Sep 17, 2021
597e8f9
minor update
chenjunjiedada Sep 22, 2021
82c8118
address comments
chenjunjiedada Sep 27, 2021
bc1a0b9
update definition accroding to proposal
chenjunjiedada Oct 4, 2021
dee5962
address comments
chenjunjiedada Oct 16, 2021
70b7162
add partition filter and minor doc updates
chenjunjiedada Oct 18, 2021
dcb3201
address more comments
chenjunjiedada Oct 20, 2021
a6a2540
remove partition filter
chenjunjiedada Oct 22, 2021
6d0853b
minor words update
chenjunjiedada Oct 26, 2021
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
55 changes: 55 additions & 0 deletions api/src/main/java/org/apache/iceberg/actions/RewriteDeletes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* 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> {

@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.

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.
 */

@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.

nit: RewriteDeletes -> RewriteDeleteFiles to match RewriteDataFiles for consistency?

@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.

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);

@chenjunjiedada chenjunjiedada Sep 17, 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.

That sounds reasonable to me. Will do.


/**
* rewrite the equality deletes.
*
* @return this for method chaining
*/
RewriteDeletes rewriteEqDeletes();

@yyanyy yyanyy Jul 28, 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.

Curious on the reason for having rewriteEqDeletes and rewritePosDeletes here, looks like in RewriteDataFiles we define methods based on the exact strategy to use, and for rewriting delete files there could be strategies like eqDelete->posDelete, combine posDelete into one, and even take both delete and data files and return data files (I think the last one should be a separate interface). Here it seems like the user can decide whether to rewrite certain type of delete files, but I would think strategy might be the thing that the user want to control more? Sorry I've been away from the community conversations for a while so might lack a lot of context if this was previously discussed.

@chenjunjiedada chenjunjiedada Jul 30, 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.

rewriteEqDeletes and rewritePosDeletes is a top-level choice for users to choose which kind of delete he or she wants to rewrite. Here's an example: actionsProvider().rewriteDeletes(table).rewriteEqDeletes().execute();, users could clearly know what kind of deletes are handled. But you remind me that, we might also don't need these two as well, use one strategy API to select its implementation is enough.

The strategy options are open and we could pass options from options interface from the specified strategy implementation.


/**
* rewrite the position deletes.
*
* @return this for method chaining
*/
RewriteDeletes rewritePosDeletes();

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

/**
* Returns the added delete files.
*/
Set<DeleteFile> addedFiles();
}
}
51 changes: 51 additions & 0 deletions core/src/main/java/org/apache/iceberg/actions/RewriteDeleteStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* 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 org.apache.iceberg.DeleteFile;
import org.apache.iceberg.Table;

public interface RewriteDeleteStrategy {

@jackye1995 jackye1995 Jul 19, 2021 •
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.

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

@chenjunjiedada chenjunjiedada Jul 20, 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.

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?

@jackye1995 jackye1995 Jul 20, 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.

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 table being modified by this rewrite strategy
*/
Table table();

/**
* Select the deletes to rewrite.
*
* @return iterable of original delete file to be replaced.
*/
Iterable<DeleteFile> selectDeletes();

@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.

could you explain a bit about how these 2 interfaces will be used? For RewriteStrategy the method Iterable<FileScanTask> selectFilesToRewrite(Iterable<FileScanTask> dataFiles) was quite straightforward, but here the method has no arguments so I am not very sure how would it be used to actually select deletes.

I think it goes to the fundamental question of what are some potential rewrite delete strategies? For rewrite data files we know there are well-known operations like bin-packing. For deletes, what are some strategies we plan to implement?

@chenjunjiedada chenjunjiedada Jul 20, 2021 •
edited
Loading

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 argument of selectFilesToRewrite(Iterable<FileScanTask> dataFiles) comes from planFileGroups and the filter logic. I think we could build similar logic by storing the iterable of FileScanTask as a local variable in the concrete RewriteDeleteStrategy class, and grouping them in rewriteDeletes() API. As for filter logic, we could add it later and apply it in selectDeletes() API.

As for strategies, it depends on different scenarios. In our cases, which mostly use HDFS as storage, we care more about the small files. they could impact the planning, reading performance and also bring overhead to the name node. So the strategy we want badly is to merge deletes. So we are planning two strategies at first: 1) merge all position deletes. 2) read all equality deletes under the partition and convert them into one position delete. I think the first one is kind of bin-packing, the second one is convert-and-bin-packing. In the future, we might also want to sort the merged position delete by file name and break it into pieces so that executors could read only the delete contents that match the data files.

@jackye1995 jackye1995 Jul 21, 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.

Thanks for the explanation. The case you described are also what I am interested in. I think for RewriteStrategy we first had a design doc so when there was the PR everything was in place. I am fine if we are taking incremental progress for delete strategies to gradually evolve the interface and add configuration options.

@openinx openinx Jul 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @chenjunjiedada , thanks for tracking the v2's compaction for long time, It's really challenging. I think we reviewers were distracted by other internal or external things , which blocked the v2 compaction progress for a long time. While the fact is: the current v2 compaction feature is the biggest blocker for v2 right now, I'd like to push this feature forward, so that we don't block v2 forever. (And thanks for your bandwidth @jackye1995, you always come out to help when the things get stuck).

About this PR, since it's a API definition, I'd like to take a look the whole delete rewrite design or preview PR if you have one because in that case I could evaluate whether the API abstraction is perfectly matching the real implementation. Of course, we could introduce a basic API, and then iterate the interface design when we get more implementation. It's looks better for me to reach a global agreement before we starting to provide a specific design, that can help us reduce context sync on the core paths.

@jackye1995 jackye1995 Jul 26, 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.

+1 for a general design. And please let me know if there is anything that can be parallelized for implementation once we reached an agreement on this. I am currently mostly spending time trying to push the work on Trino side, but should have some bandwidth to take some implementation work for this if necessary. As openinx said, this is the biggest blocker for v2.

@chenjunjiedada chenjunjiedada Jul 26, 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.

I don't have a design doc right now, but most of the ideas come from discussions in #2216 and #2364. I'm working on rebasing #2216 to apply #2372 (API definition) and #2841 (delete row reader). That would be a full implementation and hopefully could give you the full picture of how these APIs are used. I will post in the following days.

@jackye1995 jackye1995 Jul 26, 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.

great, let me read through those, thank you!


/**
* Define how to rewrite the deletes.
*
* @return iterable of delete files used to replace the original delete files.
*/
Iterable<DeleteFile> rewriteDeletes();

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

@rdblue rdblue Sep 13, 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.

Nit: it would be nice to use the same method order as RewriteStrategy for consistency.

}