openinx commented on a change in pull request #1663: URL: https://github.com/apache/iceberg/pull/1663#discussion_r526657886
########## File path: core/src/main/java/org/apache/iceberg/ContentFileWriter.java ########## @@ -0,0 +1,53 @@ +/* + * 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; + +import java.io.Closeable; +import java.util.Iterator; + +public interface ContentFileWriter<T, R> extends Closeable { Review comment: In my throught, the whole write workflow should be in the following: ``` TaskWriter | | | -------------------------------------------------- | | | | V V DeltaWriter DeltaWriter (Partition-1) (Partition-2) | | ------------------------------------------------ | | | | | | V V V RollingFileWriter RollingFileWriter RollingFileWriter (Pos-Delete) (Insert) (Equality-Delete) | | V ----------------------------- | | | | | | | | | V V V FileAppender FileAppender ... (closed) (Openning) ``` For each executor/task in compute engine, it have a TaskWriter to write generic record. If it use the fanout policy to write records then it will have multiple DeltaWriters and each one will write records for a single partition, while if use the grouped polciy in spark then we might just have one DeltaWriter in TaskWriter. The DeltaWriter could accept both INSERT/EQ-DELETE/POS-DELETE records, each kind of record we will have a RollingFileWriter which will roll its file appender to a newly opened file appender once its size reach the threshold. In the RollingFileWriter, we should have the same logic. So in theory it's good to define an abstracted ContentFileWriter so that we don't have to define three kinds of RollingFileWriter. Another way is to define a BaseRollingFileWriter and put the common logic there, then the DeltaWriter would use the BaseRollingFileWriter. when constructing the DeltaWriter, we would need to pass those subclasses PosDeleteRollingFileWriter, EqDeleteRollingFileWriter, DataRollingFileWriter to 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
