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 2021/11/02 23:54:42 UTC
[GitHub] [iceberg] jackye1995 opened a new pull request #3454: Core: add delete option for bin-packing
jackye1995 opened a new pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454
Add an option `min-deletes-per-file` to allow rewriting files with certain number of deletes. This does not remove the deletes. It is used to solve the situation that a file is already of optimized size and never included in for bin packing, but deletes are associated with it so the deletes cannot be expired.
--
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] kbendick commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742308856
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
Review comment:
Nit: Would it be worth while to add inline comments to explain these parameters (like `createTablePartitioned(4 /* ??? */, 2 /* ??? */)`? Right now, it's hard to tell immediately what these arguments are, but I'll leave the choice to you.
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -33,6 +33,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.assertj.core.util.Lists;
Review comment:
Question / Nit: Is there a reason to use `Lists` from `assertj` instead of `Lists` from the relocated google commons library?
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
Review comment:
Nit: Would it be worth while to add inline comments to explain these parameters (like `createTablePartitioned(4 /* ??? */, 2 /* ??? */)`? Right now, it's hard to tell immediately what these arguments are, but I'll leave the choice to you.
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -33,6 +33,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.assertj.core.util.Lists;
Review comment:
Question / Nit: Is there a reason to use `Lists` from `assertj` instead of `Lists` from the relocated google commons library?
--
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] kbendick commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742308856
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
Review comment:
Nit: Would it be worth while to add inline comments to explain these parameters (like `createTablePartitioned(4 /* ??? */, 2 /* ??? */)`? Right now, it's hard to tell immediately what these arguments are, but I'll leave the choice to you.
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -33,6 +33,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.assertj.core.util.Lists;
Review comment:
Question / Nit: Is there a reason to use `Lists` from `assertj` instead of `Lists` from the relocated google commons library?
--
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] jackye1995 merged pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454
--
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] jackye1995 commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742192695
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think the exact value to set depends on the user's tolerance of read performance, because more deletes means worse read performance and potentially getting out of memory, so users can tune this value based on their system requirements.
--
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] jackye1995 merged pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454
--
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] RussellSpitzer commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741622389
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think it's fine to just do this based on the number of files since the read penalty is directly related to the number of files and less so to the amount of actual rows deleted.
No strong feeling on the default since we already have the amount of delete files in the task information so I don't think the check is very expensive
--
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] jackye1995 commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742191322
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think that is equivalent to setting this value to 0?
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think the exact value to set depends on the user's tolerance of read performance, because more deletes means worse read performance and potentially getting out of memory, so users can tune this value based on their system requirements.
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -33,6 +33,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.assertj.core.util.Lists;
Review comment:
oh nice catch, should not use that. We need to add it to checkstyle.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
Review comment:
I think this is not needed, because you can see the meaning of the parameters in Intellij
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think that is equivalent to setting this value to 0?
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think the exact value to set depends on the user's tolerance of read performance, because more deletes means worse read performance and potentially getting out of memory, so users can tune this value based on their system requirements.
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -33,6 +33,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.assertj.core.util.Lists;
Review comment:
oh nice catch, should not use that. We need to add it to checkstyle.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
Review comment:
I think this is not needed, because you can see the meaning of the parameters in Intellij
--
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 change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741606736
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
What value is recommended to the user? Or how does the user compute the proper value? I'm thinking maybe adding an option to count filegroup valid if any file of it contains deletes. Because you don't know how many records match the equality delete, for example, delete a set of records in an area/province.
Nit: This is disabled by default but it is always comparing.
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
Also, do we need to have specific settings for equality delete and position delete separately? Since the read penalties are 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 change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741606736
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
What value is recommended to the user? Or how does the user compute the proper value? I'm thinking maybe adding an option to count filegroup valid if any file of it contains deletes. Because you don't know how many records match the equality delete, for example, delete a set of records in an area/province.
Nit: This is disabled by default but it is always comparing.
--
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] jackye1995 commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742191322
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think that is equivalent to setting this value to 0?
--
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] jackye1995 commented on pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#issuecomment-961449718
Looks like there are a few approvals here. Given that @RussellSpitzer who implemented this part of the code approved, I will wait until EOD today, if there is no additional comments, I will merge this, thanks everyone.
--
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] jackye1995 commented on pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#issuecomment-958456463
@RussellSpitzer @aokolnychyi @chenjunjiedada @rdblue
--
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 change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741606736
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
What value is recommended to the user? Or how does the user compute the proper value? I'm thinking maybe adding an option to count filegroup valid if any file of it contains deletes. Because you don't know how many records match the equality delete, for example, delete a set of records in an area/province.
Nit: This is disabled by default but it is always comparing.
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
Also, do we need to have specific settings for equality delete and position delete separately? Since the read penalties are 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 change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742477311
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
> I think that is equivalent to setting this value to 0?
Hm, that's it.
--
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] jackye1995 commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741539664
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
+ table.updateProperties().set(TableProperties.FORMAT_VERSION, "2").commit();
+ shouldHaveFiles(table, 8);
+ table.refresh();
+
+ CloseableIterable<FileScanTask> tasks = table.newScan().planFiles();
+ List<DataFile> dataFiles = Lists.newArrayList(CloseableIterable.transform(tasks, FileScanTask::file));
+ GenericAppenderFactory appenderFactory = new GenericAppenderFactory(table.schema(), table.spec(),
+ null, null, null);
+ int total = (int) dataFiles.stream().mapToLong(ContentFile::recordCount).sum();
+
+ RowDelta rowDelta = table.newRowDelta();
+ // remove 2 rows for odd files, 1 row for even files
+ for (int i = 0; i < dataFiles.size(); i++) {
+ DataFile dataFile = dataFiles.get(i);
+ EncryptedOutputFile outputFile = EncryptedFiles.encryptedOutput(
+ table.io().newOutputFile(table.locationProvider().newDataLocation(UUID.randomUUID().toString())),
+ EncryptionKeyMetadata.EMPTY);
+ PositionDeleteWriter<Record> posDeleteWriter = appenderFactory.newPosDeleteWriter(
+ outputFile, FileFormat.PARQUET, dataFile.partition());
+ posDeleteWriter.delete(dataFile.path(), 0);
+ posDeleteWriter.close();
+ rowDelta.addDeletes(posDeleteWriter.toDeleteFile());
+
+ if (i % 2 != 0) {
+ outputFile = EncryptedFiles.encryptedOutput(
+ table.io().newOutputFile(table.locationProvider().newDataLocation(UUID.randomUUID().toString())),
+ EncryptionKeyMetadata.EMPTY);
+ posDeleteWriter = appenderFactory.newPosDeleteWriter(outputFile, FileFormat.PARQUET, dataFile.partition());
+ posDeleteWriter.delete(dataFile.path(), 1);
+ posDeleteWriter.close();
+ rowDelta.addDeletes(posDeleteWriter.toDeleteFile());
+ }
Review comment:
I know there are some repeated code here for generating deletes. So far I am still not sure what is the correct boundary to create util methods. I am planning to refactor after I add more tests for the `RewriteDeleteStrategy`
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think that is equivalent to setting this value to 0?
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think the exact value to set depends on the user's tolerance of read performance, because more deletes means worse read performance and potentially getting out of memory, so users can tune this value based on their system requirements.
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -33,6 +33,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.assertj.core.util.Lists;
Review comment:
oh nice catch, should not use that. We need to add it to checkstyle.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
Review comment:
I think this is not needed, because you can see the meaning of the parameters in Intellij
--
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] flyxu1991 removed a comment on pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
flyxu1991 removed a comment on pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#issuecomment-965071365
Hi, i use spark3.0.3 to rewrite datafiles with this pr. but i get a err like this:
> org.apache.spark.sql.connector.catalog.CatalogNotFoundException: Catalog 'default_iceberg' plugin class not found: spark.sql.catalog.default_iceberg is not defined
at org.apache.spark.sql.connector.catalog.Catalogs$.load(Catalogs.scala:51)
at org.apache.spark.sql.connector.catalog.CatalogManager.$anonfun$catalog$1(CatalogManager.scala:52)
ERROR BaseRewriteDataFilesSparkAction: Cannot complete rewrite, partial-progress.enabled is not enabled and one of the file set groups failed to be rewritten. This error occurred during the writing of new files, not during the commit process
Here is my code:
> SparkSession spark = SparkSession
.builder()
.appName("small_file_rewrite_user_rcc")
.config("spark.sql.extensions", "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions")
.config("spark.sql.catalog.hadoop_catalog", "org.apache.iceberg.spark.SparkSessionCatalog")
.config("spark.sql.catalog.hadoop_catalog.type", "hadoop")
.config("spark.sql.catalog.hadoop_catalog.warehouse", basePath)
.getOrCreate();
HadoopCatalog hadoopCatalog = new HadoopCatalog(spark.sparkContext().hadoopConfiguration(), basePath);
TableIdentifier tableIdentifier = TableIdentifier.of("hadoop_database", "zx_user_rcc_realtime");
Table userRccTable = hadoopCatalog.loadTable(tableIdentifier);
System.out.println(userRccTable.location());
SparkActions.get(spark)
.rewriteDataFiles(userRccTable)
.option(BinPackStrategy.MIN_FILE_SIZE_BYTES, "0")
.option(RewriteDataFiles.TARGET_FILE_SIZE_BYTES, 512 * 1024 * 1024 + "")
.option(BinPackStrategy.MIN_DELETES_PER_FILE, "1")
.execute();
spark.close();
It's there something wrong with my code or something i missed?
>
--
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] jackye1995 commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742314978
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
Review comment:
I think this is not needed, because you can see the meaning of the parameters in Intellij
--
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] jackye1995 commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742314326
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -33,6 +33,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.assertj.core.util.Lists;
Review comment:
oh nice catch, should not use that. We need to add it to checkstyle.
--
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] jackye1995 commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741539664
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
+ table.updateProperties().set(TableProperties.FORMAT_VERSION, "2").commit();
+ shouldHaveFiles(table, 8);
+ table.refresh();
+
+ CloseableIterable<FileScanTask> tasks = table.newScan().planFiles();
+ List<DataFile> dataFiles = Lists.newArrayList(CloseableIterable.transform(tasks, FileScanTask::file));
+ GenericAppenderFactory appenderFactory = new GenericAppenderFactory(table.schema(), table.spec(),
+ null, null, null);
+ int total = (int) dataFiles.stream().mapToLong(ContentFile::recordCount).sum();
+
+ RowDelta rowDelta = table.newRowDelta();
+ // remove 2 rows for odd files, 1 row for even files
+ for (int i = 0; i < dataFiles.size(); i++) {
+ DataFile dataFile = dataFiles.get(i);
+ EncryptedOutputFile outputFile = EncryptedFiles.encryptedOutput(
+ table.io().newOutputFile(table.locationProvider().newDataLocation(UUID.randomUUID().toString())),
+ EncryptionKeyMetadata.EMPTY);
+ PositionDeleteWriter<Record> posDeleteWriter = appenderFactory.newPosDeleteWriter(
+ outputFile, FileFormat.PARQUET, dataFile.partition());
+ posDeleteWriter.delete(dataFile.path(), 0);
+ posDeleteWriter.close();
+ rowDelta.addDeletes(posDeleteWriter.toDeleteFile());
+
+ if (i % 2 != 0) {
+ outputFile = EncryptedFiles.encryptedOutput(
+ table.io().newOutputFile(table.locationProvider().newDataLocation(UUID.randomUUID().toString())),
+ EncryptionKeyMetadata.EMPTY);
+ posDeleteWriter = appenderFactory.newPosDeleteWriter(outputFile, FileFormat.PARQUET, dataFile.partition());
+ posDeleteWriter.delete(dataFile.path(), 1);
+ posDeleteWriter.close();
+ rowDelta.addDeletes(posDeleteWriter.toDeleteFile());
+ }
Review comment:
I know there are some repeated code here for generating deletes. So far I am still not sure what is the correct boundary to create util methods. I am planning to refactor after I add more tests for the `RewriteDeleteStrategy`
--
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] jackye1995 commented on pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#issuecomment-958456463
@RussellSpitzer @aokolnychyi @chenjunjiedada @rdblue
--
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] RussellSpitzer commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741622389
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think it's fine to just do this based on the number of files since the read penalty is directly related to the number of files and less so to the amount of actual rows deleted.
No strong feeling on the default since we already have the amount of delete files in the task information so I don't think the check is very expensive
--
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 pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#issuecomment-962681328
@jackye1995, thanks for getting this done! Sorry I didn't have a chance to review it sooner.
My only comment is that I don't think that we are using "min" in a consistent way so the option names may be confusing. For file sizes, min and max are the min and max _allowed_ file sizes. Anything that is larger than max or less than min gets rewritten. But min deletes per file is actually the minimum before taking action. In other words, it is the _maximum allowed_ number of delete files.
I'd prefer making these consistent and using `max-delete-files` instead of `min-deletes-per-file`. That fixes the min/max issue and also is more clear that we are talking about delete files and not actual deletes in the files.
--
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 change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741606736
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
What value is recommended to the user? Or how does the user compute the proper value? I'm thinking maybe adding an option to count filegroup valid if any file of it contains deletes. Because you don't know how many records match the equality delete, for example, delete a set of records in an area/province.
Nit: This is disabled by default but it is always comparing.
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
Also, do we need to have specific settings for equality delete and position delete separately? Since the read penalties are 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] kbendick commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r742308856
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestNewRewriteDataFilesAction.java
##########
@@ -177,6 +187,59 @@ public void testBinPackWithFilter() {
assertEquals("Rows must match", expectedRecords, actualRecords);
}
+ @Test
+ public void testBinPackWithDeletes() throws Exception {
+ Table table = createTablePartitioned(4, 2);
Review comment:
Nit: Would it be worth while to add inline comments to explain these parameters (like `createTablePartitioned(4 /* ??? */, 2 /* ??? */)`? Right now, it's hard to tell immediately what these arguments are, but I'll leave the choice to you.
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -33,6 +33,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.assertj.core.util.Lists;
Review comment:
Question / Nit: Is there a reason to use `Lists` from `assertj` instead of `Lists` from the relocated google commons library?
--
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] jackye1995 commented on pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#issuecomment-961298893
@aokolnychyi any additional comments? Otherwise I think this is mostly ready to be merged
--
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] jackye1995 merged pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454
--
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] RussellSpitzer commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741622389
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think it's fine to just do this based on the number of files since the read penalty is directly related to the number of files and less so to the amount of actual rows deleted.
No strong feeling on the default since we already have the amount of delete files in the task information so I don't think the check is very expensive
--
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 change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r744316266
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBinPackStrategy.java
##########
@@ -150,6 +167,23 @@ public void testGroupingMinInputFilesValid() {
ImmutableList.of(testFiles), grouped);
}
+ @Test
+ public void testGroupingWithDeletes() {
+ RewriteStrategy strategy = defaultBinPack().options(ImmutableMap.of(
+ BinPackStrategy.MIN_INPUT_FILES, Integer.toString(5),
+ BinPackStrategy.MAX_FILE_SIZE_BYTES, Long.toString(550 * MB),
+ BinPackStrategy.MIN_FILE_SIZE_BYTES, Long.toString(490 * MB),
+ BinPackStrategy.MIN_DELETES_PER_FILE, Integer.toString(2)
Review comment:
@kbendick, can you check our checkstyle config? It looks like lines that have incorrect indentation are getting through.
--
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 change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741648138
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
Also, do we need to have specific settings for equality delete and position delete separately? Since the read penalties are 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] RussellSpitzer commented on a change in pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#discussion_r741622389
##########
File path: core/src/main/java/org/apache/iceberg/actions/BinPackStrategy.java
##########
@@ -75,7 +75,20 @@
public static final String MAX_FILE_SIZE_BYTES = "max-file-size-bytes";
public static final double MAX_FILE_SIZE_DEFAULT_RATIO = 1.80d;
+ /**
+ * The minimum number of deletes that needs to be associated with a data file for it to be considered for rewriting.
+ * If a data file has more than this number of deletes, it will be rewritten regardless of its file size determined
+ * by {@link #MIN_FILE_SIZE_BYTES} and {@link #MAX_FILE_SIZE_BYTES}.
+ * If a file group contains a file that satisfies this condition, the file group will be rewritten regardless of
+ * the number of files in the file group determined by {@link #MIN_INPUT_FILES}
+ * <p>
+ * Defaults to Integer.MAX_VALUE, which means this feature is not enabled by default.
+ */
+ public static final String MIN_DELETES_PER_FILE = "min-deletes-per-file";
Review comment:
I think it's fine to just do this based on the number of files since the read penalty is directly related to the number of files and less so to the amount of actual rows deleted.
No strong feeling on the default since we already have the amount of delete files in the task information so I don't think the check is very expensive
--
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] jackye1995 commented on pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#issuecomment-961298893
--
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] flyxu1991 commented on pull request #3454: Core: add delete option for bin-packing
Posted by GitBox <gi...@apache.org>.
flyxu1991 commented on pull request #3454:
URL: https://github.com/apache/iceberg/pull/3454#issuecomment-965071365
Hi, i use spark3.0.3 to rewrite datafiles with this pr. but i get a err like this:
> org.apache.spark.sql.connector.catalog.CatalogNotFoundException: Catalog 'default_iceberg' plugin class not found: spark.sql.catalog.default_iceberg is not defined
at org.apache.spark.sql.connector.catalog.Catalogs$.load(Catalogs.scala:51)
at org.apache.spark.sql.connector.catalog.CatalogManager.$anonfun$catalog$1(CatalogManager.scala:52)
ERROR BaseRewriteDataFilesSparkAction: Cannot complete rewrite, partial-progress.enabled is not enabled and one of the file set groups failed to be rewritten. This error occurred during the writing of new files, not during the commit process
Here is my code:
> SparkSession spark = SparkSession
.builder()
.appName("small_file_rewrite_user_rcc")
.config("spark.sql.extensions", "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions")
.config("spark.sql.catalog.hadoop_catalog", "org.apache.iceberg.spark.SparkSessionCatalog")
.config("spark.sql.catalog.hadoop_catalog.type", "hadoop")
.config("spark.sql.catalog.hadoop_catalog.warehouse", basePath)
.getOrCreate();
HadoopCatalog hadoopCatalog = new HadoopCatalog(spark.sparkContext().hadoopConfiguration(), basePath);
TableIdentifier tableIdentifier = TableIdentifier.of("hadoop_database", "zx_user_rcc_realtime");
Table userRccTable = hadoopCatalog.loadTable(tableIdentifier);
System.out.println(userRccTable.location());
SparkActions.get(spark)
.rewriteDataFiles(userRccTable)
.option(BinPackStrategy.MIN_FILE_SIZE_BYTES, "0")
.option(RewriteDataFiles.TARGET_FILE_SIZE_BYTES, 512 * 1024 * 1024 + "")
.option(BinPackStrategy.MIN_DELETES_PER_FILE, "1")
.execute();
spark.close();
It's there something wrong with my code or something i missed?
>
--
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