gaborkaszab commented on code in PR #16184:
URL: https://github.com/apache/iceberg/pull/16184#discussion_r3182512540
##########
core/src/main/java/org/apache/iceberg/PartitionsTable.java:
##########
@@ -102,16 +109,7 @@ public TableScan newScan() {
@Override
public Schema schema() {
if (unpartitionedTable) {
- return schema.select(
- "record_count",
- "file_count",
- "total_data_file_size_in_bytes",
- "position_delete_record_count",
- "position_delete_file_count",
- "equality_delete_record_count",
- "equality_delete_file_count",
- "last_updated_at",
- "last_updated_snapshot_id");
+ return TypeUtil.selectNot(schema, ImmutableSet.of(PARTITION_COLUMN_ID,
SPEC_ID_COLUMN_ID));
Review Comment:
While the new version is shorter, I found it more readable to spell out what
is included instead of what isn't
##########
core/src/main/java/org/apache/iceberg/PartitionsTable.java:
##########
@@ -37,6 +39,10 @@
/** A {@link Table} implementation that exposes a table's partitions as rows.
*/
public class PartitionsTable extends BaseMetadataTable {
+ // partition dependent field id
Review Comment:
I find it weird to introduce IDs for only 2 of the schema fields. I see the
motivation that these are the ones referenced later, but still seems weird.
Maybe, would introducing each of the fields (not the IDs but the actual
fields) as NestedField members of the class and then add these fields to the
schema. With this we could get the fieldIds directly from the NestedField
similarly to what you do in FileCleanupStrategy.
--
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]
