the-other-tim-brown commented on code in PR #653: URL: https://github.com/apache/incubator-xtable/pull/653#discussion_r1958872310
########## xtable-api/src/main/java/org/apache/xtable/model/storage/FileType.java: ########## @@ -0,0 +1,45 @@ +/* + * 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.xtable.model.storage; + +/** + * Known types of storage files. For e.g. data file of a table, a position deletion, statistics + * file. + * + * @since 0.1 + */ +public enum FileType { + /** Files of type data contain the actual data records of the table. */ + DATA_FILE, + + /** + * Files of type deletion contain information of soft deleted records of the table, typically + * containing ordinal references + */ + DELETION_FILE, + + /** + * Files of type statistics typically contain supplemental statistics information related to a + * table, its partitions, or data files + */ + STATISTICS_FILE, + + /** Files of type index contain information related to the indexes of a table */ + INDEX_FILE Review Comment: Until we have a case for these, let's hold off on adding them? ########## xtable-api/src/main/java/org/apache/xtable/model/storage/InternalDataFile.java: ########## @@ -33,17 +38,18 @@ * * @since 0.1 */ -@Builder(toBuilder = true) -@Value -public class InternalDataFile { - // physical path of the file (absolute with scheme) - @NonNull String physicalPath; +@SuperBuilder(toBuilder = true) +@FieldDefaults(makeFinal = true, level = lombok.AccessLevel.PRIVATE) +@ToString(callSuper = true) +@EqualsAndHashCode(callSuper = true) +@Accessors(fluent = true) +@Getter +public class InternalDataFile extends InternalBaseFile { // file format @Builder.Default @NonNull FileFormat fileFormat = FileFormat.APACHE_PARQUET; // partition ranges for the data file @Builder.Default @NonNull List<PartitionValue> partitionValues = Collections.emptyList(); - long fileSizeBytes; long recordCount; Review Comment: Should the count be moved to the base as well? there is `countRecordsDeleted` mentioned in the RFC ########## xtable-api/src/main/java/org/apache/xtable/model/storage/InternalBaseFile.java: ########## @@ -0,0 +1,67 @@ +/* + * 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.xtable.model.storage; + +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NonNull; +import lombok.ToString; +import lombok.experimental.Accessors; +import lombok.experimental.FieldDefaults; +import lombok.experimental.SuperBuilder; + +/** + * This class is an internal representation of a logical storage file of a table and is the base + * class of all types of storage files. The most common type of storage file is a data-file, which + * contains the actual records of a table. Other examples of a storage file are positional delete + * files (containing ordinals of the records to be deleted from a data file), stat files, and index + * files. For completeness of the conversion process, XTable needs to recognize current storage + * files of a table, and also the storage files that are added and removed over time as the state of + * the table changes. Different table formats support different storage file types. This base file + * representation is generic and extensible and is needed to design stable interfaces. + */ +@SuperBuilder(toBuilder = true) +@FieldDefaults(makeFinal = true, level = lombok.AccessLevel.PRIVATE) +@ToString(callSuper = true) +@EqualsAndHashCode +@AllArgsConstructor +@Accessors(fluent = true) +@Getter +public abstract class InternalBaseFile { + // Absolute path of the storage file, with the scheme, that contains this logical file. Typically, + // one physical storage file contains only one base file, for e.g. a parquet data file. However, + // in some cases, one storage file can contain multiple logical storage files for optimizations. + @NonNull String physicalPath; + + // The offset of the logical file in the physical storage file. + @Builder.Default long offset = 0; Review Comment: How would this apply to a data file? It may be confusing to put this in the base if there is no use for it. ########## xtable-api/src/main/java/org/apache/xtable/model/storage/FilesDiff.java: ########## @@ -92,12 +92,12 @@ public static <L, P> FilesDiff<L, P> findNewAndRemovedFiles( * @param <P> the type of the previous files * @return the set of files that are added */ - public static <P> FilesDiff<InternalDataFile, P> findNewAndRemovedFiles( + public static <P> FilesDiff<? extends InternalBaseFile, P> findNewAndRemovedFiles( Review Comment: Would it be less verbose to define some interface for `InternalFile` that we can use instead of this wildcard? ########## xtable-api/src/main/java/org/apache/xtable/model/storage/InternalDataFile.java: ########## @@ -33,17 +38,18 @@ * * @since 0.1 */ -@Builder(toBuilder = true) -@Value -public class InternalDataFile { - // physical path of the file (absolute with scheme) - @NonNull String physicalPath; +@SuperBuilder(toBuilder = true) +@FieldDefaults(makeFinal = true, level = lombok.AccessLevel.PRIVATE) +@ToString(callSuper = true) +@EqualsAndHashCode(callSuper = true) +@Accessors(fluent = true) Review Comment: What is the reason for switching to fluent accessors instead of the get prefix? -- 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: commits-unsubscr...@xtable.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
