gaborkaszab commented on code in PR #17134: URL: https://github.com/apache/iceberg/pull/17134#discussion_r3552241731
########## core/src/main/java/org/apache/iceberg/MetadataTables.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.util.Arrays; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.types.Types; + +/** A {@link Table} implementation that exposes a table's metadata tables as rows. */ +public class MetadataTables extends BaseMetadataTable { + MetadataTables(Table table) { + this(table, table.name() + "." + MetadataTableType.METADATA_TABLES.tableName()); + } + + MetadataTables(Table table, String name) { + super(table, name); + } + + private static final Schema METADATA_TABLES_SCHEMA = + new Schema(Types.NestedField.required(1, "metadata_table", Types.StringType.get())); Review Comment: "metadata_table_name" ? ########## core/src/main/java/org/apache/iceberg/MetadataTables.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.util.Arrays; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.types.Types; + +/** A {@link Table} implementation that exposes a table's metadata tables as rows. */ +public class MetadataTables extends BaseMetadataTable { + MetadataTables(Table table) { + this(table, table.name() + "." + MetadataTableType.METADATA_TABLES.tableName()); + } + + MetadataTables(Table table, String name) { + super(table, name); + } + + private static final Schema METADATA_TABLES_SCHEMA = + new Schema(Types.NestedField.required(1, "metadata_table", Types.StringType.get())); + + @Override + public TableScan newScan() { + return new MetadataTableScan(table(), schema()); + } + + @Override + public Schema schema() { + return METADATA_TABLES_SCHEMA; + } + + @Override + MetadataTableType metadataTableType() { + return MetadataTableType.METADATA_TABLES; + } + + private DataTask task(BaseTableScan scan) { + StaticDataTask.Row[] rows = + Arrays.stream(MetadataTableType.values()) + .map(metadataType -> StaticDataTask.Row.of(metadataType.tableName())) + .toArray(StaticDataTask.Row[]::new); + + DataFile metadataFile = + DataFiles.builder(PartitionSpec.unpartitioned()) + .withPath(table().location() + "#" + MetadataTableType.METADATA_TABLES.tableName()) + .withFormat(FileFormat.METADATA) + .withRecordCount(MetadataTableType.values().length) + .withFileSizeInBytes(0) + .build(); + + return new StaticDataTask(metadataFile, schema(), scan.schema(), rows); Review Comment: Other metadata tables use `StaticDataTask.of()` ########## core/src/main/java/org/apache/iceberg/MetadataTables.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.util.Arrays; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.types.Types; + +/** A {@link Table} implementation that exposes a table's metadata tables as rows. */ +public class MetadataTables extends BaseMetadataTable { + MetadataTables(Table table) { + this(table, table.name() + "." + MetadataTableType.METADATA_TABLES.tableName()); + } + + MetadataTables(Table table, String name) { + super(table, name); + } + + private static final Schema METADATA_TABLES_SCHEMA = + new Schema(Types.NestedField.required(1, "metadata_table", Types.StringType.get())); + + @Override + public TableScan newScan() { + return new MetadataTableScan(table(), schema()); + } + + @Override + public Schema schema() { + return METADATA_TABLES_SCHEMA; + } + + @Override + MetadataTableType metadataTableType() { + return MetadataTableType.METADATA_TABLES; + } + + private DataTask task(BaseTableScan scan) { + StaticDataTask.Row[] rows = + Arrays.stream(MetadataTableType.values()) + .map(metadataType -> StaticDataTask.Row.of(metadataType.tableName())) + .toArray(StaticDataTask.Row[]::new); + + DataFile metadataFile = + DataFiles.builder(PartitionSpec.unpartitioned()) + .withPath(table().location() + "#" + MetadataTableType.METADATA_TABLES.tableName()) + .withFormat(FileFormat.METADATA) + .withRecordCount(MetadataTableType.values().length) + .withFileSizeInBytes(0) + .build(); + + return new StaticDataTask(metadataFile, schema(), scan.schema(), rows); + } + + private class MetadataTableScan extends BaseMetadataTableScan { Review Comment: Instead of `MetadataTableScan` maybe `MetadataTablesScan` or `MetadataTableNameScan` ########## core/src/main/java/org/apache/iceberg/MetadataTables.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.util.Arrays; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.types.Types; + +/** A {@link Table} implementation that exposes a table's metadata tables as rows. */ +public class MetadataTables extends BaseMetadataTable { + MetadataTables(Table table) { + this(table, table.name() + "." + MetadataTableType.METADATA_TABLES.tableName()); + } + + MetadataTables(Table table, String name) { + super(table, name); + } + + private static final Schema METADATA_TABLES_SCHEMA = + new Schema(Types.NestedField.required(1, "metadata_table", Types.StringType.get())); + + @Override + public TableScan newScan() { + return new MetadataTableScan(table(), schema()); + } + + @Override + public Schema schema() { + return METADATA_TABLES_SCHEMA; + } + + @Override + MetadataTableType metadataTableType() { + return MetadataTableType.METADATA_TABLES; + } + + private DataTask task(BaseTableScan scan) { + StaticDataTask.Row[] rows = + Arrays.stream(MetadataTableType.values()) + .map(metadataType -> StaticDataTask.Row.of(metadataType.tableName())) + .toArray(StaticDataTask.Row[]::new); + + DataFile metadataFile = + DataFiles.builder(PartitionSpec.unpartitioned()) + .withPath(table().location() + "#" + MetadataTableType.METADATA_TABLES.tableName()) + .withFormat(FileFormat.METADATA) + .withRecordCount(MetadataTableType.values().length) + .withFileSizeInBytes(0) + .build(); + + return new StaticDataTask(metadataFile, schema(), scan.schema(), rows); + } + + private class MetadataTableScan extends BaseMetadataTableScan { + MetadataTableScan(Table table, Schema schema) { Review Comment: I don't think any of these constructors need the Schema parameter. See `RefsTable.RefsTableScan` ########## core/src/main/java/org/apache/iceberg/MetadataTables.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.util.Arrays; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.types.Types; + +/** A {@link Table} implementation that exposes a table's metadata tables as rows. */ +public class MetadataTables extends BaseMetadataTable { + MetadataTables(Table table) { + this(table, table.name() + "." + MetadataTableType.METADATA_TABLES.tableName()); + } + + MetadataTables(Table table, String name) { + super(table, name); + } + + private static final Schema METADATA_TABLES_SCHEMA = + new Schema(Types.NestedField.required(1, "metadata_table", Types.StringType.get())); + + @Override + public TableScan newScan() { + return new MetadataTableScan(table(), schema()); + } + + @Override + public Schema schema() { + return METADATA_TABLES_SCHEMA; + } + + @Override + MetadataTableType metadataTableType() { + return MetadataTableType.METADATA_TABLES; + } + + private DataTask task(BaseTableScan scan) { + StaticDataTask.Row[] rows = Review Comment: Checking `RefsTable` instead of constructing `StaticDataTask.Row[]', we can simply use `Collection<String>` with a transform function to convert it to row via `StaticDataTask.Row.of()` ########## core/src/main/java/org/apache/iceberg/MetadataTables.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.util.Arrays; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.types.Types; + +/** A {@link Table} implementation that exposes a table's metadata tables as rows. */ +public class MetadataTables extends BaseMetadataTable { + MetadataTables(Table table) { + this(table, table.name() + "." + MetadataTableType.METADATA_TABLES.tableName()); + } + + MetadataTables(Table table, String name) { + super(table, name); + } + + private static final Schema METADATA_TABLES_SCHEMA = + new Schema(Types.NestedField.required(1, "metadata_table", Types.StringType.get())); + + @Override + public TableScan newScan() { + return new MetadataTableScan(table(), schema()); + } + + @Override + public Schema schema() { + return METADATA_TABLES_SCHEMA; + } + + @Override + MetadataTableType metadataTableType() { + return MetadataTableType.METADATA_TABLES; + } + + private DataTask task(BaseTableScan scan) { + StaticDataTask.Row[] rows = + Arrays.stream(MetadataTableType.values()) + .map(metadataType -> StaticDataTask.Row.of(metadataType.tableName())) + .toArray(StaticDataTask.Row[]::new); + + DataFile metadataFile = Review Comment: I randomly checked other metadata tables and they seem to use the following InputFile: `table().io().newInputFile(table().operations().current().metadataFileLocation()),` ########## core/src/main/java/org/apache/iceberg/MetadataTables.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.util.Arrays; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.types.Types; + +/** A {@link Table} implementation that exposes a table's metadata tables as rows. */ +public class MetadataTables extends BaseMetadataTable { + MetadataTables(Table table) { + this(table, table.name() + "." + MetadataTableType.METADATA_TABLES.tableName()); + } + + MetadataTables(Table table, String name) { + super(table, name); + } + + private static final Schema METADATA_TABLES_SCHEMA = + new Schema(Types.NestedField.required(1, "metadata_table", Types.StringType.get())); + + @Override + public TableScan newScan() { + return new MetadataTableScan(table(), schema()); + } + + @Override + public Schema schema() { + return METADATA_TABLES_SCHEMA; + } + + @Override + MetadataTableType metadataTableType() { + return MetadataTableType.METADATA_TABLES; + } + + private DataTask task(BaseTableScan scan) { + StaticDataTask.Row[] rows = + Arrays.stream(MetadataTableType.values()) + .map(metadataType -> StaticDataTask.Row.of(metadataType.tableName())) + .toArray(StaticDataTask.Row[]::new); + + DataFile metadataFile = + DataFiles.builder(PartitionSpec.unpartitioned()) + .withPath(table().location() + "#" + MetadataTableType.METADATA_TABLES.tableName()) + .withFormat(FileFormat.METADATA) + .withRecordCount(MetadataTableType.values().length) + .withFileSizeInBytes(0) + .build(); + + return new StaticDataTask(metadataFile, schema(), scan.schema(), rows); + } + + private class MetadataTableScan extends BaseMetadataTableScan { + MetadataTableScan(Table table, Schema schema) { + super(table, schema, MetadataTableType.METADATA_TABLES); + } + + MetadataTableScan(Table table, Schema schema, TableScanContext context) { + super(table, schema, MetadataTableType.METADATA_TABLES, context); + } + + @Override + protected TableScan newRefinedScan(Table table, Schema schema, TableScanContext context) { + return new MetadataTableScan(table, schema, context); + } + + @Override + protected CloseableIterable<FileScanTask> doPlanFiles() { + return CloseableIterable.withNoopClose(task(this)); + } + } Review Comment: After reding this Scan class and comparing to others can't we simply do what `PartitionsScan` does? ``` PartitionsScan(Table table) { super( table, PartitionsTable.this.schema(), MetadataTableType.PARTITIONS, PartitionsTable.this::task); ``` ########## spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java: ########## @@ -483,10 +496,14 @@ public void testAllFilesPartitioned() throws Exception { Schema entriesTableSchema = TypeUtil.selectNot( - Spark3Util.loadIcebergTable(spark, tableName + ".entries").schema(), + Spark3Util.loadIcebergTable( + spark, tableName + "." + MetadataTableType.ENTRIES.tableName()) + .schema(), Set.of(DataFile.FIRST_ROW_ID.fieldId())); Schema filesTableSchema = - Spark3Util.loadIcebergTable(spark, tableName + ".all_data_files").schema(); + Spark3Util.loadIcebergTable( Review Comment: These changes are pure refactors. Move to separate PR to reduce the noise as advised above. ########## core/src/test/java/org/apache/iceberg/hadoop/TestTableSerialization.java: ########## @@ -194,7 +194,11 @@ public void testSerializableMetadataTablesPlanning(boolean fromSerialized) throw Set<CharSequence> newFiles = getFiles(getMetaDataTable(table, type)); // Expect that the new data is changed in the meantime - assertThat(deserializedFiles).isNotEqualTo(newFiles); + if (type == MetadataTableType.METADATA_TABLES) { Review Comment: not entirely sure why the new table is any different than the other in this test. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
