You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2025/07/29 08:37:01 UTC
[PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
nastra opened a new pull request, #13695:
URL: https://github.com/apache/iceberg/pull/13695
This preserves the original type of the upper/lower bound of a particular field metric.
This will be later used to convert from `Metrics` to the new content stats
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2240305480
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestVariantMetrics.java:
##########
@@ -140,6 +140,9 @@ public void testShreddedPrimitiveTypes(VariantValue value) throws IOException {
return bounds.get(ROOT_FIELD);
})
.isEqualTo(value);
+
+ assertThat(metrics.originalTypes().get(1)).isEqualTo(Types.LongType.get());
+ assertThat(metrics.originalTypes().get(2)).isEqualTo(Types.VariantType.get());
Review Comment:
no reason, just an oversight. Comparing with a map is definitely shorter, thanks
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2252714096
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -151,6 +152,7 @@ public static class Builder {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
Ok this makes sense to me now, I had missed for the DV case that the change to `FileMetadata` would already cover propagation of the types required for the future stats conversion (since we just copy over the fields from the metrics in the builder)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2240316922
##########
api/src/main/java/org/apache/iceberg/Metrics.java:
##########
@@ -37,6 +38,7 @@ public class Metrics implements Serializable {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
yep good idea
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2247624930
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -151,6 +152,7 @@ public static class Builder {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
> @nastra: Let me rephrase what I understand from your comment:
>
> * Currently we have Metrics - here in some cases we have some binary data for min and max which is not typed
currently lower/upper bounds are **all** binary and we don't know what their original type was
> * In the future we will have only Stats - where the stats will be typed, and this type will help interpret the min/max values
yes the new stats structure will store upper/lower bound with their actual type
>
> In between, for a while will have Metrics converted to Stats, and for this we need the type info
correct. We want to convert from metrics to stats and we can only do so if we know the original type of the upper/lower bound.
One of the reasons why I'm doing the metrics -> stats conversion is because currently our appender/writer APIs are all returning `Metrics` after data has been written. Changing those APIs to return stats instead would be quite a big change, which I want to avoid until we figure out how we want the new API to look like
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2241787634
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -151,6 +152,7 @@ public static class Builder {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
I don't think we would want to do this in `BaseFile`, since that would mean we would expose an `originalTypes()` method there, which isn't needed. For `DataFiles` we do it because most places call `withMetrics()` and we want to preserve the original types when we later start converting from `Metrics` to the new structure
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#issuecomment-3153586229
thanks for reviewing @pvary and @amogh-jahagirdar, I'll get this merged
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #13695:
URL: https://github.com/apache/iceberg/pull/13695
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2243714790
##########
api/src/main/java/org/apache/iceberg/Metrics.java:
##########
@@ -74,6 +77,25 @@ public Metrics(
this.upperBounds = upperBounds;
}
+ public Metrics(
+ Long rowCount,
+ Map<Integer, Long> columnSizes,
+ Map<Integer, Long> valueCounts,
+ Map<Integer, Long> nullValueCounts,
+ Map<Integer, Long> nanValueCounts,
+ Map<Integer, ByteBuffer> lowerBounds,
+ Map<Integer, ByteBuffer> upperBounds,
+ Map<Integer, Type> originalTypes) {
+ this.rowCount = rowCount;
+ this.columnSizes = columnSizes;
+ this.valueCounts = valueCounts;
+ this.nullValueCounts = nullValueCounts;
+ this.nanValueCounts = nanValueCounts;
+ this.lowerBounds = lowerBounds;
+ this.upperBounds = upperBounds;
+ this.originalTypes = originalTypes;
Review Comment:
Nit: Shall we update the existing constructors to call `this(...)` and pass in nulls for the extraneous arguments?
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -151,6 +152,7 @@ public static class Builder {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
So I think the principle here is that anywhere we need to build new `DataFile` or `DeleteFile` metadata we need to make sure that the metrics original types information is copied over so that it can be used when converting to the future structure. The only place that really needs read exposure to the `originalTypes` is the Metrics class itself.
But I think I agree with @pvary, I do think the field does need to be copied over (we don't need to expose it neccesarily). I think the field needs to be copied over for the DeleteFile case since DeleteFile extends BaseFile and we could have stats for those that we would want to convert
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2251647921
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -151,6 +152,7 @@ public static class Builder {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
Discussed with @nastra, to move the `Metrics.originalTypes()` to package private. So we don't pollute the API with this method. This way we don't have to describe the users when this field is available, and when it is not available.
I think this is a good compromise, so we can proceed with implementing the stats, and eventually deprecate the way to generate the stats from metrics.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2240442431
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -151,6 +152,7 @@ public static class Builder {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
Do we need to copy this in the `BaseFile` too?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2243714374
##########
api/src/main/java/org/apache/iceberg/Metrics.java:
##########
@@ -74,6 +77,25 @@ public Metrics(
this.upperBounds = upperBounds;
}
+ public Metrics(
+ Long rowCount,
+ Map<Integer, Long> columnSizes,
+ Map<Integer, Long> valueCounts,
+ Map<Integer, Long> nullValueCounts,
+ Map<Integer, Long> nanValueCounts,
+ Map<Integer, ByteBuffer> lowerBounds,
+ Map<Integer, ByteBuffer> upperBounds,
+ Map<Integer, Type> originalTypes) {
+ this.rowCount = rowCount;
+ this.columnSizes = columnSizes;
+ this.valueCounts = valueCounts;
+ this.nullValueCounts = nullValueCounts;
+ this.nanValueCounts = nanValueCounts;
+ this.lowerBounds = lowerBounds;
+ this.upperBounds = upperBounds;
+ this.originalTypes = originalTypes;
Review Comment:
Nit: Shall we update the existing constructors to call `this(...)` and pass in nulls for the extraneous arguments?
##########
api/src/main/java/org/apache/iceberg/Metrics.java:
##########
@@ -74,6 +77,25 @@ public Metrics(
this.upperBounds = upperBounds;
}
+ public Metrics(
+ Long rowCount,
+ Map<Integer, Long> columnSizes,
+ Map<Integer, Long> valueCounts,
+ Map<Integer, Long> nullValueCounts,
+ Map<Integer, Long> nanValueCounts,
+ Map<Integer, ByteBuffer> lowerBounds,
+ Map<Integer, ByteBuffer> upperBounds,
+ Map<Integer, Type> originalTypes) {
+ this.rowCount = rowCount;
+ this.columnSizes = columnSizes;
+ this.valueCounts = valueCounts;
+ this.nullValueCounts = nullValueCounts;
+ this.nanValueCounts = nanValueCounts;
+ this.lowerBounds = lowerBounds;
+ this.upperBounds = upperBounds;
+ this.originalTypes = originalTypes;
Review Comment:
Nit: Shall we update the existing constructors to call `this(...)` and pass in nulls for the extraneous arguments?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2244705118
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -151,6 +152,7 @@ public static class Builder {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
> So I think the principle here is that anywhere we need to build new `DataFile` or `DeleteFile` metadata we need to make sure that the metrics original types information is copied over so that it can be used when converting to the future structure. The only place that really needs read exposure to the `originalTypes` is the Metrics class itself.
Yes that's absolutely correct. The main use case is that we want to convert from a `Metrics` instance to a Stats instance when e.g. an appender/writer creates a new `Metrics` instance and before those metrics are written to disk. Also we don't want to store `originalTypes` on disk, since that would require a spec change and we wouldn't have that information anyway when reading existing data files.
> But I think I agree with @pvary, I do think the field does need to be copied over (we don't need to expose it neccesarily). I think the field needs to be copied over for the DeleteFile case since DeleteFile extends BaseFile and we could have stats for those that we would want to convert
I took another look at this but I'm still not sure why we would want to carry forward the `originalTypes` in `BaseFile`. There's really only a single use case I could find in the codebase that would require exposing `originalTypes()` in order to carry it forward and that is in `RewriteTablePathUtil` when a delete manifest is rewritten. The metrics of that `DeleteFile` are carried forward in https://github.com/apache/iceberg/blob/62d9ff5d043a5571efe020b9177998ae763a41a0/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java#L491-L492 which might indicate that we would need to preserve `originalTypes` there as well, but the `DeleteFile` was read from the delete manifest at that point and we don't actually store `originalTypes` on disk, meaning that it would be `null` anyway.
That being said, the `Metrics` class is our main surface area where we want to preserve `originalTypes`. Once we keep individual metrics fields and don't create a new `Metrics` instance again from those fields (which is the case with `BaseFile` - except `RewriteTablePathUtil`), we shouldn't need to preserve `originalTypes` anymore.
@pvary @amogh-jahagirdar let me know if that makes sense or whether I'm missing something obvious
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2245828253
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -151,6 +152,7 @@ public static class Builder {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
@nastra: Let me rephrase what I understand from your comment:
- Currently we have Metrics - here in some cases we have some binary data for min and max which is not typed
- In the future we will have only Stats - where the stats will be typed, and this type will help interpret the min/max values
In between, for a while will have Metrics converted to Stats, and for this we need the type info
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2240227612
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestVariantMetrics.java:
##########
@@ -140,6 +140,9 @@ public void testShreddedPrimitiveTypes(VariantValue value) throws IOException {
return bounds.get(ROOT_FIELD);
})
.isEqualTo(value);
+
+ assertThat(metrics.originalTypes().get(1)).isEqualTo(Types.LongType.get());
+ assertThat(metrics.originalTypes().get(2)).isEqualTo(Types.VariantType.get());
Review Comment:
Any reason not using?
```
assertThat(metrics.originalTypes()).isEqualTo(Map.of(1, Types.LongType.get(), 2, Types.VariantType.get()));
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Preserve original Type for upper/lower bounds in Metrics [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13695:
URL: https://github.com/apache/iceberg/pull/13695#discussion_r2240234765
##########
api/src/main/java/org/apache/iceberg/Metrics.java:
##########
@@ -37,6 +38,7 @@ public class Metrics implements Serializable {
private Map<Integer, Long> nanValueCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ private Map<Integer, Type> originalTypes = null;
Review Comment:
Do we want to comment somewhere that the `originalTypes` are not serialized?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org