You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "pvary (via GitHub)" <gi...@apache.org> on 2025/02/18 10:08:33 UTC
[PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
pvary opened a new pull request, #12306:
URL: https://github.com/apache/iceberg/pull/12306
Compaction interface changes which separates the planning and the execution phase of the compaction.
This enables moving the common planning part of the compaction from Spark code to the core, and Flink (or another execution engines) could implement their own rewrite executor to do the actual compaction.
See discussion on: https://lists.apache.org/thread/6lj2jn3dbvqjscc96w0mc32bhxq0qfqv
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1965214008
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long maxOutputFileSize() {
+ return maxOutputFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
Review Comment:
Reworded. Please take a look.
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long maxOutputFileSize() {
+ return maxOutputFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
+ * files. The final split size is adjusted to be at least as big as the target file size but less
+ * than the max write file size.
+ */
+ public long inputSplitSize() {
+ return inputSplitSize;
+ }
+
+ /** Expected number of the output files. */
Review Comment:
Changed
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1960230031
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
Review Comment:
it is confusing to introduce `FileRewriteGroup` name with another existing class named `RewriteFileGroup`.
We can probably call this `BaseFileGroup` or `BaseRewriteFileGroup`.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963878608
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlanner.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for planning content file rewrites.
Review Comment:
Planning needs more defining here imho.
The notes below are not clear to me. They seem to be for the entire Operation. I think if we were going to write that there it should be something like
A Rewrite Operation is performed by several classes working in conjunction.
1) A FileRewritePlanner - (this class) which scans the files in the table and determines which files should be rewritten and how they should be grouped. This produces a FileRewritePlan.
2) FileRewritePlan - This immutable output of the planner contains a set of Groups, each containing a set of files that are ment to be compacted together by the FileRewriteRunner.
3) The FileRewrite Runner is the engine specific implementation that can take a FileRewritePlan and perform a rewrite on each of the groups within. This is the actual implementation of the rewrite which generates new 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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963939995
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
Review Comment:
This is specific to the planner not the Group
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu merged PR #12306:
URL: https://github.com/apache/iceberg/pull/12306
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961247530
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for rewriting content file groups ({@link FileRewriteGroup}). The lifecycle for the
+ * executor looks like the following:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the executor with the configuration parameters
+ * <li>{@link #rewrite(FileRewriteGroup)} called for every group in the plan to do the actual
+ * rewrite of the files, and returns the generated new files.
+ * </ul>
+ *
+ * A single executor could be used to rewrite multiple groups for the same plan.
+ *
+ * @param <FGI> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
+ * RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ * @param <G> the Java type of the planned groups
+ */
+public interface FileRewriteExecutor<
+ FGI,
Review Comment:
I didn't change this, because the info objects are coming from the immutable base infrastructure for the actions. I decided against touching those to limit the scope of 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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1962168871
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
Review Comment:
I see. there is a compatibility issue with inheritance. agree we can keep it as it is
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963950250
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long maxOutputFileSize() {
+ return maxOutputFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
+ * files. The final split size is adjusted to be at least as big as the target file size but less
+ * than the max write file size.
+ */
+ public long inputSplitSize() {
+ return inputSplitSize;
+ }
+
+ /** Expected number of the output files. */
Review Comment:
The total number of files that should be produced by the rewrite of this entire file group.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961275441
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
+ return fileScanTasks;
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long writeMaxFileSize() {
+ return writeMaxFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
+ * files. The final split size is adjusted to be at least as big as the target file size but less
+ * than the max write file size.
+ */
+ public long splitSize() {
Review Comment:
Kept the current method name, but it's ok to rename 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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963861306
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlan.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.CloseableIterable;
+
+/**
+ * Result of the file rewrite planning as generated by the {@link FileRewritePlanner#plan()}.
+ *
+ * <p>The plan contains an iterable for the planned groups and statistics about the number of the
Review Comment:
The doc here is a bit confusing to me. I think it has a bit too many implementation details.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1960255226
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for rewriting content file groups ({@link FileRewriteGroup}). The lifecycle for the
+ * executor looks like the following:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the executor with the configuration parameters
+ * <li>{@link #rewrite(FileRewriteGroup)} called for every group in the plan to do the actual
+ * rewrite of the files, and returns the generated new files.
+ * </ul>
+ *
+ * A single executor could be used to rewrite multiple groups for the same plan.
+ *
+ * @param <FGI> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
+ * RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ * @param <G> the Java type of the planned groups
+ */
+public interface FileRewriteExecutor<
Review Comment:
nit: In Spark, `Executor` is meant specially for worker process. wondering if we can come up with a different name. maybe `FileRewriteRunner` that we can borrow from the Beam concept: https://beam.apache.org/documentation/basics/#runner?
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#issuecomment-2686570993
yes, current state looks good to me. let me merge 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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1965208153
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlan.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.CloseableIterable;
+
+/**
+ * Result of the file rewrite planning as generated by the {@link FileRewritePlanner#plan()}.
+ *
+ * <p>The plan contains an iterable for the planned groups and statistics about the number of the
Review Comment:
Fixed
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlanner.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for planning content file rewrites.
Review Comment:
Fixed, please take a look
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#issuecomment-2688197241
Thanks @stevenzwu and @RussellSpitzer!
I will be OOO next week, so the next PR will come after that
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1960273582
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for rewriting content file groups ({@link FileRewriteGroup}). The lifecycle for the
+ * executor looks like the following:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the executor with the configuration parameters
+ * <li>{@link #rewrite(FileRewriteGroup)} called for every group in the plan to do the actual
+ * rewrite of the files, and returns the generated new files.
+ * </ul>
+ *
+ * A single executor could be used to rewrite multiple groups for the same plan.
+ *
+ * @param <FGI> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
Review Comment:
minor suggestions on the Javadoc
```
* @param <FGI> the Java type of the rewrite file group info like {@link RewriteDataFiles.FileGroupInfo} or {@link RewritePositionDeleteFiles.FileGroupInfo}
* @param <T> the Java type of the input scan tasks (input)
* @param <F> the Java type of the content files (input and output)
* @param <G> the Java type of the rewrite file group like {@link ...}
```
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
+ return fileScanTasks;
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long writeMaxFileSize() {
+ return writeMaxFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
+ * files. The final split size is adjusted to be at least as big as the target file size but less
+ * than the max write file size.
+ */
+ public long splitSize() {
+ return splitSize;
+ }
+
+ /** Expected number of the output files. */
+ public int expectedOutputFiles() {
+ return expectedOutputFiles;
+ }
+
+ /** Accumulated size for the input files. */
+ public long sizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int numFiles() {
Review Comment:
nit: `numInputFiles`
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for rewriting content file groups ({@link FileRewriteGroup}). The lifecycle for the
+ * executor looks like the following:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the executor with the configuration parameters
+ * <li>{@link #rewrite(FileRewriteGroup)} called for every group in the plan to do the actual
+ * rewrite of the files, and returns the generated new files.
+ * </ul>
+ *
+ * A single executor could be used to rewrite multiple groups for the same plan.
+ *
+ * @param <FGI> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
+ * RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ * @param <G> the Java type of the planned groups
+ */
+public interface FileRewriteExecutor<
+ FGI,
+ T extends ContentScanTask<F>,
+ F extends ContentFile<F>,
+ G extends FileRewriteGroup<FGI, T, F>> {
+
+ /** Returns a description for this rewriter. */
+ default String description() {
+ return getClass().getName();
+ }
+
+ /**
+ * Returns a set of supported options for this rewriter. Only options specified in this list will
+ * be accepted at runtime. Any other options will be rejected.
+ */
+ Set<String> validOptions();
+
+ /**
+ * Initializes this rewriter using provided options.
Review Comment:
rewriter -> runner
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for rewriting content file groups ({@link FileRewriteGroup}). The lifecycle for the
+ * executor looks like the following:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the executor with the configuration parameters
+ * <li>{@link #rewrite(FileRewriteGroup)} called for every group in the plan to do the actual
+ * rewrite of the files, and returns the generated new files.
+ * </ul>
+ *
+ * A single executor could be used to rewrite multiple groups for the same plan.
+ *
+ * @param <FGI> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
+ * RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ * @param <G> the Java type of the planned groups
+ */
+public interface FileRewriteExecutor<
+ FGI,
+ T extends ContentScanTask<F>,
+ F extends ContentFile<F>,
+ G extends FileRewriteGroup<FGI, T, F>> {
+
+ /** Returns a description for this rewriter. */
+ default String description() {
+ return getClass().getName();
+ }
+
+ /**
+ * Returns a set of supported options for this rewriter. Only options specified in this list will
Review Comment:
nit: rewriter -> runner
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for rewriting content file groups ({@link FileRewriteGroup}). The lifecycle for the
+ * executor looks like the following:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the executor with the configuration parameters
+ * <li>{@link #rewrite(FileRewriteGroup)} called for every group in the plan to do the actual
+ * rewrite of the files, and returns the generated new files.
+ * </ul>
+ *
+ * A single executor could be used to rewrite multiple groups for the same plan.
+ *
+ * @param <FGI> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
+ * RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ * @param <G> the Java type of the planned groups
+ */
+public interface FileRewriteExecutor<
+ FGI,
Review Comment:
Right now, it seems that {@link RewriteDataFiles.FileGroupInfo} and {@link RewritePositionDeleteFiles.FileGroupInfo} interfaces are identical. wondering if we should start without generics for `FGI`? it can affect future flexibility though.
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
+ return fileScanTasks;
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long writeMaxFileSize() {
+ return writeMaxFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
+ * files. The final split size is adjusted to be at least as big as the target file size but less
+ * than the max write file size.
+ */
+ public long splitSize() {
+ return splitSize;
+ }
+
+ /** Expected number of the output files. */
+ public int expectedOutputFiles() {
+ return expectedOutputFiles;
+ }
+
+ /** Accumulated size for the input files. */
+ public long sizeInBytes() {
Review Comment:
`inputFilesSizeInBytes`?
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
Review Comment:
nit: I know the method name is copied from existing code. Since this is a new class, we have an opportunity to name it more accurately as `scanTasks` or `fileScanTasks`.
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
+ return fileScanTasks;
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long writeMaxFileSize() {
+ return writeMaxFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
+ * files. The final split size is adjusted to be at least as big as the target file size but less
+ * than the max write file size.
+ */
+ public long splitSize() {
Review Comment:
inputSplitSize?
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
Review Comment:
for consistency with the class above, should this generic type also be called `FGI`?
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
Review Comment:
info -> fileGroupInfo
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
+ return fileScanTasks;
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long writeMaxFileSize() {
Review Comment:
maxWriteFileSize or maxOutputFileSize?
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
Review Comment:
nit: maybe just `scan tasks for input files`?
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlanner.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for planning content file rewrites.
+ *
+ * <p>The entire rewrite operation is broken down into pieces. The grouping is based on partitioning
+ * and the planning could create multiple groups within a partition. As a result {@link
+ * FileRewritePlan} is generated which contains the data need by the {@link FileRewriteExecutor}s
+ * which execute the actual file rewrite.
+ *
+ * <p>The lifecycle of the planner is:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the planner with the configuration parameters
+ * <li>{@link #plan()} generates the plan for the given configuration
+ * </ul>
+ *
+ * @param <FGI> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
+ * RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ * @param <G> the Java type of the planned groups
+ */
+public interface FileRewritePlanner<
+ FGI,
+ T extends ContentScanTask<F>,
+ F extends ContentFile<F>,
+ G extends FileRewriteGroup<FGI, T, F>> {
+
+ /** Returns a description for this rewriter. */
+ default String description() {
+ return getClass().getName();
+ }
+
+ /**
+ * Returns a set of supported options for this rewriter. Only options specified in this list will
Review Comment:
rewriter 0> planner
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlan.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.CloseableIterable;
+
+/**
+ * Result of the file rewrite planning as generated by the {@link FileRewritePlanner#plan()}.
+ *
+ * <p>The plan contains an iterable for the planned groups and statistics about the number of the
+ * generated groups, like the total number of the groups and the groups per partition. The plan also
+ * contains some calculated values required by the {@link FileRewriteExecutor}s where the values are
+ * based on the input data and the planning parameters.
+ *
+ * <p>Groups in a plan could be processed independently. For example, in Spark this means that each
+ * group would be rewritten in its own Spark job.
+ *
+ * @param <I> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
+ * RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ * @param <G> the Java type of the planned groups
+ */
+public class FileRewritePlan<
+ I,
Review Comment:
`FGI` for consistency?
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#issuecomment-2669440254
@pvary regarding the inheritance, I am thinking it might still be valuable to rename/clarify them in the new base class. We can still keep and deprecate the existing methods and just forward the calls to the base class. some of the method names aren't as clear, like `info`, `numFiles`. We can use this refactoring opportunity to improve them.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#issuecomment-2669661834
> @pvary regarding the inheritance, I am thinking it might still be valuable to rename/clarify them in the new base class. We can still keep and deprecate the existing methods and just forward the calls to the base class. some of the method names aren't as clear, like `info`, `numFiles`. We can use this refactoring opportunity to improve them.
Ok. Will do. I agree but wanted to start from the minimal changeset.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961276060
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
+ return fileScanTasks;
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long writeMaxFileSize() {
Review Comment:
Kept the current method name, but it's ok to rename 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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963878608
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlanner.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for planning content file rewrites.
Review Comment:
Planning needs more defining here imho.
The notes below are not clear to me. They seem to be for the entire Operation. I think if we were going to write that there it should be something like
A Rewrite Operation is performed by several classes working in conjunction.
1) A FileRewritePlanner - (this class) which scans the files in the table and determines which files should be rewritten and how they should be grouped. This produces a FileRewritePlan.
2) FileRewritePlan - This immutable output of the planner contains a set of Groups, each containing a set of files that are ment to be compacted together by the FileRewriteRunner.
3) The FileRewrite Runner is the engine specific implementation that can take a FileRewritePlan and perform a rewrite on each of the groups within.
?
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963948180
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long maxOutputFileSize() {
+ return maxOutputFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
Review Comment:
This should be something like
``` The amount of bytes of data the FileRewriteRunner should pull from a single group in a single read task."
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963948180
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long maxOutputFileSize() {
+ return maxOutputFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
Review Comment:
This should be something like
The amount of bytes of data the FileRewriteRunner should pull from a single group in a single read task (?)
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963939995
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
Review Comment:
This doc is specific to the planner not the Group
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961253836
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
Review Comment:
This checkstyle module prevents using FGI for classes:
```
<module name="ClassTypeParameterName"> <!-- Java Style Guide: Type variable names -->
<property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/>
</module>
```
We don't have similar for interfaces, like:
```
<module name="InterfaceTypeParameterName"> <!-- Java Style Guide: Type variable names -->
<property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/>
</module>
```
Since it was not highlighted by checkstyle, I forgot to update the interfaces. I will move from `FGI` to `I` as well.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963944890
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long maxOutputFileSize() {
+ return maxOutputFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
Review Comment:
Also specific to the planner. I'm also not sure if split size is a good name here.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961267361
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
Review Comment:
This method exists in the current API as `RewriteFileGroup.fileScans()` and `RewritePositionDeletesGroup.tasks()` and the new method name will be inherited from the `FileRewriteGroup`
Minimally one of them needs to be deprecated, renamed. Do you think we should change both?
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1962170427
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
Review Comment:
ok. didn't see the inheritance earlier. probably not worth change 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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1962312132
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
Review Comment:
Currently only action base classes are there with name starting with base.
`RewriteFilePlan` will be the final class. I don't plan to add different plans for different compaction methods. They will only differ in the `RewriteFileGroup`s.
What about the planner/runner interfaces? They are definitely not `base`
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1962199091
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
Review Comment:
> Names staring with Base are reserved for the immutable interfaces for the actions themselves.
Is `Base*` reserved only for actions in that package? Personally, I don't see an issue of adding classes like `BaseRewriteFileGroup`, `BaseRewriteFilePlan` etc.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963868582
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlan.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.CloseableIterable;
+
+/**
+ * Result of the file rewrite planning as generated by the {@link FileRewritePlanner#plan()}.
+ *
+ * <p>The plan contains an iterable for the planned groups and statistics about the number of the
Review Comment:
Probably something like
a Container class holding a the output of a FileRewritePlanner.plan call. Contains a list of "RewriteFileGroups" see ... link which are used by engines to rewrite a particular set of 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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1963879716
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteRunner.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for rewriting content file groups ({@link RewriteGroupBase}). The lifecycle for the
+ * runner looks like the following:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the runner with the configuration parameters
+ * <li>{@link #rewrite(RewriteGroupBase)} called for every group in the plan to do the actual
+ * rewrite of the files, and returns the generated new files.
+ * </ul>
+ *
+ * A single runner could be used to rewrite multiple groups for the same plan.
Review Comment:
Is this note required?
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1962170427
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
Review Comment:
ok. didn't see the inheritance earlier. probably not worth change 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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961274346
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
+ return fileScanTasks;
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long writeMaxFileSize() {
+ return writeMaxFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
+ * files. The final split size is adjusted to be at least as big as the target file size but less
+ * than the max write file size.
+ */
+ public long splitSize() {
+ return splitSize;
+ }
+
+ /** Expected number of the output files. */
+ public int expectedOutputFiles() {
+ return expectedOutputFiles;
+ }
+
+ /** Accumulated size for the input files. */
+ public long sizeInBytes() {
Review Comment:
This method exists in the current API as `RewriteFileGroup.sizeInBytes()` and `RewritePositionDeletesGroup.rewrittenBytes()` and the new method name will be inherited from the FileRewriteGroup
Minimally one of them needs to be deprecated, renamed. Do you think we should change both?
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961277287
##########
core/src/main/java/org/apache/iceberg/actions/FileRewritePlan.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.CloseableIterable;
+
+/**
+ * Result of the file rewrite planning as generated by the {@link FileRewritePlanner#plan()}.
+ *
+ * <p>The plan contains an iterable for the planned groups and statistics about the number of the
+ * generated groups, like the total number of the groups and the groups per partition. The plan also
+ * contains some calculated values required by the {@link FileRewriteExecutor}s where the values are
+ * based on the input data and the planning parameters.
+ *
+ * <p>Groups in a plan could be processed independently. For example, in Spark this means that each
+ * group would be rewritten in its own Spark job.
+ *
+ * @param <I> the Java type of the plan info like {@link RewriteDataFiles.FileGroupInfo} or {@link
+ * RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ * @param <G> the Java type of the planned groups
+ */
+public class FileRewritePlan<
+ I,
Review Comment:
Moved to `I` for consistency
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1965209565
##########
core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteRunner}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the input scan tasks (input)
+ * @param <F> the Java type of the content files (input and output)
+ */
+public abstract class RewriteGroupBase<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long maxOutputFileSize;
+ private final long inputSplitSize;
+ private final int expectedOutputFiles;
+
+ RewriteGroupBase(
+ I info,
+ List<T> fileScanTasks,
+ long maxOutputFileSize,
+ long inputSplitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.maxOutputFileSize = maxOutputFileSize;
+ this.inputSplitSize = inputSplitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Scan tasks for input files. */
+ public List<T> fileScanTasks() {
+ return fileScanTasks;
+ }
+
+ /** Accumulated size for the input files. */
+ public long inputFilesSizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int inputFileNum() {
+ return fileScanTasks.size();
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
Review Comment:
Reworded to highlight the relevant part for the runners. Please take a look.
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1965208778
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteRunner.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for rewriting content file groups ({@link RewriteGroupBase}). The lifecycle for the
+ * runner looks like the following:
+ *
+ * <ul>
+ * <li>{@link #init(Map)} initializes the runner with the configuration parameters
+ * <li>{@link #rewrite(RewriteGroupBase)} called for every group in the plan to do the actual
+ * rewrite of the files, and returns the generated new files.
+ * </ul>
+ *
+ * A single runner could be used to rewrite multiple groups for the same plan.
Review Comment:
Removed
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961259881
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
Review Comment:
This is a public method in RewriteFileGroup and RewritePositionDeletesGroup. With inheritance the method name is inherited too. I felt that we don't want to deprecate methods for this rename. If your feelings are strong enough to create deprecation for this, then we can change 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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961237242
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
Review Comment:
Names staring with `Base` are reserved for the immutable interfaces for the actions themselves.
I would like to keep the FileRewriteGroup/FileRewritePlan/FileRewritePlanner/FileRewriteRunner similar and/or grouped together.
Maybe create a new package (org.apache.iceberg.actions.rewrite)? Maybe a name starting with the same prefix (RewriteGroup/RewritePlan/RewritePlanner/RewriteRunner)?
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1962168871
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
Review Comment:
I see. there is a compatibility issue with inheritance. agree we can keep it as it is
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and executor [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#discussion_r1961269850
##########
core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.actions;
+
+import java.util.List;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or
+ * {@link RewritePositionDeleteFiles.FileGroupInfo}
+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {
+ private final I info;
+ private final List<T> fileScanTasks;
+ private final long writeMaxFileSize;
+ private final long splitSize;
+ private final int expectedOutputFiles;
+
+ FileRewriteGroup(
+ I info,
+ List<T> fileScanTasks,
+ long writeMaxFileSize,
+ long splitSize,
+ int expectedOutputFiles) {
+ this.info = info;
+ this.fileScanTasks = fileScanTasks;
+ this.writeMaxFileSize = writeMaxFileSize;
+ this.splitSize = splitSize;
+ this.expectedOutputFiles = expectedOutputFiles;
+ }
+
+ /** Identifiers and partition information about the group. */
+ public I info() {
+ return info;
+ }
+
+ /** Input of the group. {@link ContentScanTask}s to read. */
+ public List<T> fileScans() {
+ return fileScanTasks;
+ }
+
+ /**
+ * While we create tasks that should all be smaller than our target size, there is a chance that
+ * the actual data will end up being larger than our target size due to various factors of
+ * compression, serialization, which are outside our control. If this occurs, instead of making a
+ * single file that is close in size to our target, we would end up producing one file of the
+ * target size, and then a small extra file with the remaining data.
+ *
+ * <p>For example, if our target is 512 MB, we may generate a rewrite task that should be 500 MB.
+ * When we write the data we may find we actually have to write out 530 MB. If we use the target
+ * size while writing, we would produce a 512 MB file and an 18 MB file. If instead we use a
+ * larger size estimated by this method, then we end up writing a single file.
+ *
+ * @return the target size plus one half of the distance between max and target
+ */
+ public long writeMaxFileSize() {
+ return writeMaxFileSize;
+ }
+
+ /**
+ * Determines the reader split size as the input size divided by the desired number of output
+ * files. The final split size is adjusted to be at least as big as the target file size but less
+ * than the max write file size.
+ */
+ public long splitSize() {
+ return splitSize;
+ }
+
+ /** Expected number of the output files. */
+ public int expectedOutputFiles() {
+ return expectedOutputFiles;
+ }
+
+ /** Accumulated size for the input files. */
+ public long sizeInBytes() {
+ return fileScanTasks.stream().mapToLong(T::length).sum();
+ }
+
+ /** Number of the input files. */
+ public int numFiles() {
Review Comment:
This method exists in the current API as `RewriteFileGroup.numFiles()` and `RewritePositionDeletesGroup.numRewrittenDeleteFiles()` and the new method name will be inherited from the FileRewriteGroup
Minimally one of them needs to be deprecated, renamed. Do you think we should change both?
--
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
Re: [PR] Core: Interface changes for separating rewrite planner and runner [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #12306:
URL: https://github.com/apache/iceberg/pull/12306#issuecomment-2685588950
Ok I'm on board, I think the interfaces are good and Java Doc is solid for now, if @stevenzwu is good with the changes I say we merge and start the rest of the refactor.
--
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