szehon-ho commented on code in PR #9813:
URL: https://github.com/apache/iceberg/pull/9813#discussion_r1508313002
##########
core/src/main/java/org/apache/iceberg/BaseFilesTable.java:
##########
@@ -54,7 +56,23 @@ public Schema schema() {
schema = TypeUtil.selectNot(schema,
Sets.newHashSet(DataFile.PARTITION_ID));
}
- return TypeUtil.join(schema,
MetricsUtil.readableMetricsSchema(table().schema(), schema));
+ return withDerivedColumns(schema);
+ }
+
+ private Schema withDerivedColumns(Schema schema) {
+ AtomicInteger nextId = new AtomicInteger(schema.highestFieldId());
Review Comment:
I think we dont need the complexity of nextId and can make a constant field
id (and even make the whole NestedField into a constant to reduce code in this
method).
##########
core/src/main/java/org/apache/iceberg/MetadataTableUtils.java:
##########
@@ -109,4 +117,68 @@ public static Table createMetadataTableInstance(
private static String metadataTableName(String tableName, MetadataTableType
type) {
return tableName + (tableName.contains("/") ? "#" : ".") +
type.name().toLowerCase(Locale.ROOT);
}
+
+ static class StructWithDerivedColumns implements StructLike {
Review Comment:
Initially I think this made sense because we had only one column. But with
two, it becomes un-managable. Can we investigate whether StructProjection will
work here? Then we can remove both this and the MetricsUtil struct class.
##########
core/src/main/java/org/apache/iceberg/BaseFilesTable.java:
##########
@@ -158,14 +176,26 @@ static class ManifestReadTask extends BaseFileScanTask
implements DataTask {
@Override
public CloseableIterable<StructLike> rows() {
Types.NestedField readableMetricsField =
projection.findField(MetricsUtil.READABLE_METRICS);
+ Types.NestedField dataSequenceNumberField =
+ projection.findField(MetadataTableUtils.DATA_SEQUENCE_NUMBER);
- if (readableMetricsField == null) {
+ if (readableMetricsField == null && dataSequenceNumberField == null) {
Review Comment:
If we can reduce the number of possible branches, that would be great for
code readability.
one way is to enhance withDerviedColumns to itself be smarter, and wrap
conditionally. Then here we would just return withDerivedColumns all the time.
--
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]
