You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/05 12:37:17 UTC
[GitHub] [iceberg] chenjunjiedada opened a new pull request, #4703: Api: add option to ignore position deletes in rewrite validation
chenjunjiedada opened a new pull request, #4703:
URL: https://github.com/apache/iceberg/pull/4703
This reduces conflicts when performing rewrite service for the fink upsert job.
When streaming change log to iceberg table, the flink task writers produce position deletes against the data files that were produced in the same transaction. The offline rewrite service thus is safe to commit the rewritten data files without caring about the position deletes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#issuecomment-1130550165
Let me take a look at this one too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r886218979
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
> Suppose pos1 file references records in f1. If I understand correctly, trying to rewrite f1 and f2 would fail right now as there is a matching position delete.
This rewrite should not fail if there is no further upsert snapshot IIUC, it only fails when we have another upsert snapshot that pos deletes matches `f1` or `f2`, but in the Flink upsert job, it is impossible. @jackye1995 @RussellSpitzer can you confirm this? I think this is why we think different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#issuecomment-1139821351
Sorry for the delay. I'll take another look later today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877144799
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
How do we know whether it is safe to use this option given that Spark can produce position delete files now and the same table can be accessed by Flink and Spark?
Have we considered exposing an option to ignore position deletes only added in the same snapshot and modifying/adding a new method to `DeleteFileIndex` to cover the new matching logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877234494
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
I haven't seen #4748. I'll try to check it out later today. From a quick look, I think it is similar yet a bit different from what I was proposing here. I was thinking about a new method in `DeleteFileIndex` that would only match position delete files added in later snapshots (ignoring position deletes added in the same snapshot) and then having an option to use that from the `RewriteFiles` API.
I think our assumption should be that multiple engines can write to the table (we are about to have such use cases, in fact). I also believe it is not hard to implement and will be much more flexible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877234494
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
I haven't seen #4748. I'll try to check it out later today. From a quick look, I think it is similar yet a bit different from what I was proposing here. I was thinking about a new method in `DeleteFileIndex` that would only match position delete files added in later snapshots (ignoring position deletes added in the same snapshot) and then having an option to use that from the `RewriteFiles` API.
I think our assumption should be that multiple engines can write to the table (we are about to have such use cases, in fact).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877687193
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Let me simulate use cases, assuming we first have the following upsert commits.
<pre>
commit-1:
{seq=1, [f[1], f[2], d[1]]}
commit-2:
{seq=1, [f[1], f[2], d[1]]},
{seq=2, [f[3], f[4], d[2]]}
</pre>
With changes in this PR and #4748, we could have rewrite planning and result as follows (we don't remove deletes right now):
<pre>
plan:
task1 = {f[1], f[2], d[1]},
task2 = {f[3], f[4], d[2]}
commit-3:
{seq=1, [f[5], d[1]},
{seq=2, [f[6], d[2]]
</pre>
Now, if we produce a new commit that contains new pos deletes that target f[1] and f[2].
<pre>
commit-3:
{seq=1, [f[1], f[2], d[1]]},
{seq=2, [f[3], f[4], d[2]]},
{seq=3, [d[3]]}
</pre>
With the suggested planning strategy, the result is:
<pre>
plan:
task1 = {f[1], f[2], d[3]}
commit-4:
{seq=1, [f[1], f[2], d[1]]},
{seq=2, [f[3], f[4], d[2]]},
{seq=3, [f[5], d[3]]}
</pre>
It looks weird unless the assumption is that the sequence number of a position delete must be larger than its reference data files. If that is the case, I would suggest adding another option to DeleteFileIndex and RewriteFiles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r868765676
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,16 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Set the snapshot ID used in any reads for this operation.
+ * <p>
+ * Validations will check changes after this snapshot ID. If this is not called, all ancestor snapshots through the
+ * table's initial snapshot are validated.
+ *
+ * @param snapshotId a snapshot ID
+ * @param ignorePosDeletes whether to ignore position delete in validation
+ * @return this for method chaining
+ */
+ RewriteFiles validateFromSnapshot(long snapshotId, boolean ignorePosDeletes);
Review Comment:
Make sense to me. Let me refactor this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r868766014
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -361,10 +361,12 @@ private CloseableIterable<ManifestEntry<DataFile>> addedDataFiles(TableMetadata
* @param base table metadata to validate
* @param startingSnapshotId id of the snapshot current at the start of the operation
* @param dataFiles data files to validate have no new row deletes
+ * @param ignorePosDeletes whether position deletes should be ignored in validation
*/
protected void validateNoNewDeletesForDataFiles(TableMetadata base, Long startingSnapshotId,
- Iterable<DataFile> dataFiles) {
- validateNoNewDeletesForDataFiles(base, startingSnapshotId, null, dataFiles, newFilesSequenceNumber != null);
+ Iterable<DataFile> dataFiles, boolean ignorePosDeletes) {
+ validateNoNewDeletesForDataFiles(base, startingSnapshotId, null, dataFiles, newFilesSequenceNumber != null,
+ ignorePosDeletes);
Review Comment:
It does look weird, let me refactor this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877539023
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
+1 to @aokolnychyi's suggestion. By ignoring delete files that were written in the same commit as a data file you're checking, you can do this safely
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877167580
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Do you mean this one: https://github.com/apache/iceberg/pull/4748? I opened one week ago... As you can see in the unit test, with the option it could skip a lot of position deletes. We verified in production this PR together with #4748 could bring many performance improvements in a real upsert pipeline.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r886221365
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Do you mean to add bloom filter position deletes, right? Great to hear that. Hope we can also use min/max in manifest cache and bloom filter in the data file to remove equality deletes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r868755289
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,16 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Set the snapshot ID used in any reads for this operation.
+ * <p>
+ * Validations will check changes after this snapshot ID. If this is not called, all ancestor snapshots through the
+ * table's initial snapshot are validated.
+ *
+ * @param snapshotId a snapshot ID
+ * @param ignorePosDeletes whether to ignore position delete in validation
+ * @return this for method chaining
+ */
+ RewriteFiles validateFromSnapshot(long snapshotId, boolean ignorePosDeletes);
Review Comment:
I don't think this is the right way to configure this option. Why not add a new method since this is orthogonal to the snapshot you're validating from? Or is there a more appropriate `validate` method that could be updated instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r876439278
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
I think this method disables position delete file validation altogether that may cause correctness issues if multiple engines operate on the same table. If I understand correctly, our goal is not to fail rewrites of data files where there is a matching position delete file added in the same snapshot (i.e. Flink upsert), assuming that position delete file was applied when the data file was compacted. What if Spark added a separate position delete file for the data file produced by Flink? We can't commit the rewrite, can we?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r889071134
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
I agree with Anton's approach. I think we can automatically detect cases where we can do this optimization. That's much safer than adding a config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r886218979
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
> Suppose pos1 file references records in f1. If I understand correctly, trying to rewrite f1 and f2 would fail right now as there is a matching position delete.
This rewrite should not fail if there is no further upsert snapshaot IIUC, it only fails when we have another upsert snapshot that pos deletes matches `f1` or `f2`. @jackye1995 @RussellSpitzer can you confirm this? I think this is why we think different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r911519169
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
@aokolnychyi @rdblue Just want to double confirm, do we agree to apply bloom filtering on position delete? Applying bloom filtering on the executor side should be ok, what I'm concerned about is when planning on the driver would the bloom filtering brings heavy overhead since we have to open these small position deletes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877707563
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
I'm confused here, the Flink upsert job produces position deletes that target the data files produced in the same commit, so how can we ignore the deletes in the same commit when rewriting? Despite we can have later position deletes, we can not ignore the position delete that has the same sequence number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada closed pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada closed pull request #4703: API: Optionally ignore position deletes in rewrite validation
URL: https://github.com/apache/iceberg/pull/4703
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r886218979
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
> Suppose pos1 file references records in f1. If I understand correctly, trying to rewrite f1 and f2 would fail right now as there is a matching position delete.
This rewrite should not fail IIUC, it only fails when we have another upsert snapshot that pos deletes matches `f1` or `f2`. @jackye1995 @RussellSpitzer can you confirm this? I think this is why we think different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r885934904
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Well, I am no longer sure we are talking about the same use case. Let me give an example of what I meant.
```
snapshot 1 (upsert):
f1, f2, eq1, pos1
```
Suppose `pos1` file references records in `f1`. If I understand correctly, trying to rewrite `f1` and `f2` would fail right now as there is a matching position delete. I was proposing to add a method to `RewriteFiles` that would ignore delete files for data files added in the same snapshot. This should unlock rewriting upserts and will be safe as long as `pos1` is applied whenever `f1` and `f2` are rewritten (that's how compaction works right now).
Now what should happen if a concurrent operation is committed while we are rewriting? Suppose we have another concurrent upsert.
```
snapshot 2 (upsert):
f1, f2, eq1, pos1 (seq = 0)
f3, f4, eq2, pos2 (seq = 1)
```
With the flag I was talking about, we will discard `pos1` but we will still validate `pos2` does not match `f1` and `f2`. Right now, we will use partition info and min/max file path to see if `pos2` matches `f1` or `f2`. It can still give us false positives but we also plan to have bloom filters, which should improve filtering.
There is also another scenario which we have to avoid.
```
snapshot 2 (Spark merge-on-read on top Flink upsert):
f1, f2, eq1, pos1 (seq = 0)
f3, f4, pos2 (seq = 1)
```
In this case, `pos2` can indeed reference `f1` and `f2`. If that happens, we can't commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r885934904
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Well, I am no longer sure we are talking about the same use case. Let me give an example of what I meant.
```
snapshot 1 (upsert):
f1, f2, eq1, pos1
```
Suppose `pos1` file references records in `f1`. If I understand correctly, trying to rewrite `f1` and `f2` would fail right now as there is a matching position delete. I was proposing to add a method to `RewriteFiles` that would ignore delete files for data files added in the same snapshot. This should unlock rewriting upserts and will be safe as long as `pos1` is applied whenever `f1` and `f2` are rewritten (that's how compaction works right now).
Now what should happen if a concurrent operation is committed while we are rewriting? Suppose we have another concurrent upsert.
```
snapshot 2 (upsert):
f1, f2, eq1, pos1 (seq = 0)
f3, f4, eq2, pos2 (seq = 1)
```
With the flag I was talking about, we will discard `pos1` but we will still validate `pos2` does not match `f1` and `f2`. Right now, we will use partition info and min/max file path to see if `pos2` matches `f1` or `f2`. It can still give us false positives but we also plan to have bloom filters, which should improve filtering.
There is also another scenario which we have to avoid.
```
snapshot 2 (Spark merge-on-read on top of Flink upsert):
f1, f2, eq1, pos1 (seq = 0)
f3, f4, pos2 (seq = 1)
```
In this case, `pos2` can indeed reference `f1` and `f2`. If that happens, we can't commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r868766285
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -414,6 +423,10 @@ private void validateNoNewDeletesForDataFiles(TableMetadata base, Long startingS
ValidationException.check(
Arrays.stream(deleteFiles).noneMatch(deleteFile -> deleteFile.content() == FileContent.POSITION_DELETES),
"Cannot commit, found new position delete for replaced data file: %s", dataFile);
+ } else if (ignorePosDeletes) {
Review Comment:
Nice catch! Let me break `if/else if/else`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r886247045
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
I guess you are right that the first case is handled as `validateNoNewDeletesForDataFiles` checks only new delete files added after the snapshot we used for reading.
Could you confirm the compaction you are using sets `validateFromSnapshot` correctly? If so, then the problem is that our delete matching logic may give false positives (it is currently limited to partition and file path min/max filtering). Then your solution starts to make sense but I am still concerned that it is up to the user to say if it is safe to apply this optimization and it is not something we can do when multiple engines interact with the table.
Is there any immediate optimization we can do in the delete matching logic to reduce the number of false positives? Thoughts anyone? Bloom filtering and opening delete files if there are not that many of those are the solutions I can think of right now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r885934904
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Well, I am no longer sure we are talking about the same use case. Let me give an example of what I meant.
```
snapshot 1 (upsert):
f1, f2, eq1, pos1
```
Suppose `pos1` file references records in `f1`. If I understand correctly, trying to rewrite `f1` and `f2` would fail right now as there is a matching position delete. I was proposing to add a method to `RewriteFiles` that would ignore delete files for data files added in the same snapshot. This should unlock rewriting upserts and will be safe as long as `pos1` is applied whenever `f1` and `f2` are rewritten (that's how compaction works right now).
Now what should happen if a concurrent operation is committed while we are rewriting? Suppose we have another concurrent upsert.
```
snapshot 2 (upsert):
f1, f2, eq1, pos1 (seq = 0)
f3, f4, eq2, pos2 (seq = 1)
```
With the flag I was talking about, we will discard `pos1` but we will still validate `pos2` does not match `f1` and `f2`. Right now, we will use partition info and min/max file path to see if `pos2` matches `f1` or `f2`. It can still give us false positives but we also plan to have bloom filters, which should improve filtering.
There is also another scenario which we have to avoid.
```
snapshot 2 (spark merge-on-read):
f1, f2, eq1, pos1 (seq = 0)
f3, f4, pos2 (seq = 1)
```
In this case, `pos2` can indeed reference `f1` and `f2`. If that happens, we can't commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r911527796
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
@aokolnychyi @rdblue Can we write some metadata for position deletes that what sequence numbers of data files that it can reference since we can get exact data files sequence number when writing deletes. I mean add a API like `List<long> referedSeqOfDataFiles()`, a simple one might be `boolean hasSameSeqWithReferedDataFile()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r885936445
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
@chenjunjiedada, are you interested in the first use case (without concurrent operations) or the second one (with concurrent operations)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877178095
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Theoretically, Spark and Flink can produce deletes at the same time. While users usually use one engine to write, for example, use the Flink for upsert/CDC writes and rewrite the deletes by Spark. So I think providing an option would be better since users know how their tables are written.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r868756636
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -414,6 +423,10 @@ private void validateNoNewDeletesForDataFiles(TableMetadata base, Long startingS
ValidationException.check(
Arrays.stream(deleteFiles).noneMatch(deleteFile -> deleteFile.content() == FileContent.POSITION_DELETES),
"Cannot commit, found new position delete for replaced data file: %s", dataFile);
+ } else if (ignorePosDeletes) {
Review Comment:
Nit: extra space.
This logic looks incorrect to me. If equality deletes are not ignored, then this will always check for position deletes, regardless of the value for `ignorePosDeletes`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877167580
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Do you mean this one: https://github.com/apache/iceberg/pull/4748?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877687193
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Let me simulate use cases, assuming we first have the following upsert commits(`s` as the sequence number).
<pre>
commit-1:
{f1(s=1), f2(s=1), d1(s=1)}
commit-2:
{f1, f2, d1},
{f3(s=2), f4(s=2), d2(s=2)}
</pre>
With changes in this PR and #4748, we could have rewrite planning and result as follows (we don't remove deletes right now):
<pre>
plan:
task1 = {f1, f2, d1},
task2 = {f3, f4, d2}
commit-3:
{ f5(s=2), f6(s=2), d1, d2}
</pre>
Now, if we produce a new commit that contains new pos deletes that target f[1] and f[2].
<pre>
commit-3:
{f1, f2, d1},
{f3, f4, d2}
{d3(s=3)}
</pre>
With the suggested planning strategy, the result is:
<pre>
plan:
task1 = {f1, f2, d3}
commit-4:
{f1, f2, d1},
{f3, f4, d2}
{d3}
{f5(s=3)}
</pre>
It looks not efficient unless the assumption is that the sequence number of a position delete must be larger than its reference data files. Is that right or am I missing something here? If that is the case, I would suggest adding another option to `DeleteFileIndex` and `RewriteFiles`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r876504437
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
I considered that situation so use an option to control it. Actually, we have many unit tests that are doing the way you described, but in the production, we rarely add position deletes asynchronously AFAIK. With an option, users can control the rewrite behavior according to their real cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#issuecomment-1304453056
Replaced by https://github.com/apache/iceberg/pull/5760.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r877687193
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Let me simulate use cases, assuming we first have the following upsert commits(`s` as the sequence number).
<pre>
commit-1:
{f1(s=1), f2(s=1), d1(s=1)}
commit-2:
{f1, f2, d1},
{f3(s=2), f4(s=2), d2(s=2)}
</pre>
With changes in this PR and #4748, we could have rewrite planning and result as follows (we don't remove deletes right now):
<pre>
plan:
task1 = {f1, f2, d1},
task2 = {f3, f4, d2}
commit-3:
{ f5(s=2), f6(s=2), d1, d2}
</pre>
Now, if we produce a new commit that contains new pos deletes that target f[1] and f[2].
<pre>
commit-3:
{f1, f2, d1},
{f3, f4, d2}
{d3(s=3)}
</pre>
With the suggested planning strategy, the result is:
<pre>
plan:
task1 = {f1, f2, d3}
commit-4:
{f1, f2, d1},
{f3, f4, d2}
{d3}
{f5(s=3)}
</pre>
It looks weird unless the assumption is that the sequence number of a position delete must be larger than its reference data files. If that is the case, I would suggest adding another option to DeleteFileIndex and RewriteFiles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r886247045
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
I guess you are right that the first case is handled as `validateNoNewDeletesForDataFiles` checks only new delete files added after the snapshot we used for reading.
Could you confirm the compaction you are using sets `validateFromSnapshot` correctly? If so, then the problem is that our delete matching logic may give false positives (it is currently limited to partition and file path min/max filtering). Then your solution starts to make sense but I am still concerned that it is up to the user to say if it is safe to apply this optimization and it is not something we can do when multiple engines interact with the table.
Is there any immediate optimization we can do in the delete matching logic to reduce the number of false positives? Thoughts anyone?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r887883984
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
> Could you confirm the compaction you are using sets validateFromSnapshot correctly? If so, then the problem is that our delete matching logic may give false positives (it is currently limited to partition and file path min/max filtering). Then your solution starts to make sense but I am still concerned that it is up to the user to say if it is safe to apply this optimization and it is not something we can do when multiple engines interact with the table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r911521950
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Plus, we only support writer bloom filter on parquet format right now so it cannot support other format delete? Another way is to write bloom filters of deletes on puffin, not sure if that could work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r911524702
##########
api/src/main/java/org/apache/iceberg/RewriteFiles.java:
##########
@@ -84,4 +84,12 @@ RewriteFiles rewriteFiles(Set<DataFile> dataFilesToReplace, Set<DeleteFile> dele
* @return this for method chaining
*/
RewriteFiles validateFromSnapshot(long snapshotId);
+
+ /**
+ * Ignore the position deletes in rewrite validation. Flink upsert job only generates position deletes in the
+ * ongoing transaction, so it is not necessary to validate position deletes when rewriting.
+ *
+ * @return this for method chaining
+ */
+ RewriteFiles ignorePosDeletesInValidation();
Review Comment:
Or we can add more info when writing position deletes, such as a new field that indicate what kind of data files it includes, the data files that have the same sequence number, or the data files that have less sequence number. This should be the least cost AFAIK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#discussion_r868755932
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -361,10 +361,12 @@ private CloseableIterable<ManifestEntry<DataFile>> addedDataFiles(TableMetadata
* @param base table metadata to validate
* @param startingSnapshotId id of the snapshot current at the start of the operation
* @param dataFiles data files to validate have no new row deletes
+ * @param ignorePosDeletes whether position deletes should be ignored in validation
*/
protected void validateNoNewDeletesForDataFiles(TableMetadata base, Long startingSnapshotId,
- Iterable<DataFile> dataFiles) {
- validateNoNewDeletesForDataFiles(base, startingSnapshotId, null, dataFiles, newFilesSequenceNumber != null);
+ Iterable<DataFile> dataFiles, boolean ignorePosDeletes) {
+ validateNoNewDeletesForDataFiles(base, startingSnapshotId, null, dataFiles, newFilesSequenceNumber != null,
+ ignorePosDeletes);
Review Comment:
It doesn't seem to make sense to make one flag explicit and the other one implied by the sequence number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on pull request #4703: API: Optionally ignore position deletes in rewrite validation
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on PR #4703:
URL: https://github.com/apache/iceberg/pull/4703#issuecomment-1121907445
@rdblue , I think the change to remove position deletes references is related to this, so I put that change in the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org