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 2026/04/10 22:30:39 UTC
[PR] API: Include size unit in avg/max value size fields [iceberg]
nastra opened a new pull request, #15939:
URL: https://github.com/apache/iceberg/pull/15939
(no comment)
--
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: Include size unit in avg/max value size fields [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15939:
URL: https://github.com/apache/iceberg/pull/15939#discussion_r3069034729
##########
api/src/main/java/org/apache/iceberg/stats/FieldStatistic.java:
##########
@@ -125,13 +125,13 @@ public static Types.StructType fieldStatsFor(Types.NestedField field, int baseFi
baseFieldId + AVG_VALUE_SIZE.offset(),
AVG_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Avg value size of variable-length types (String, Binary)"));
+ "Avg value size in bytes of variable-length types (String, Binary)"));
fields.add(
optional(
baseFieldId + MAX_VALUE_SIZE.offset(),
MAX_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Max value size of variable-length types (String, Binary)"));
+ "Max value size in bytes of variable-length types (String, Binary)"));
Review Comment:
@singhpk234 apologies, I thought you had a slightly bigger change in mind (hence why I thought a separate PR is better) but I think we only need
```
- "Avg value size in bytes of variable-length types (String, Binary)"
+ "Avg stored (compressed, encoded) value size in bytes of variable-length types (String, Binary)"
- "Max value size in bytes of variable-length types (String, Binary)"
+ "Max stored (compressed, encoded) value size in bytes of variable-length types (String, Binary)"
```
I still have another PR in this area that I wanted to create, so I'll make sure to include this there, 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: Include size unit in avg/max value size fields [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #15939:
URL: https://github.com/apache/iceberg/pull/15939
--
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: Include size unit in avg/max value size fields [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15939:
URL: https://github.com/apache/iceberg/pull/15939#discussion_r3074141487
##########
api/src/main/java/org/apache/iceberg/stats/FieldStatistic.java:
##########
@@ -125,13 +125,13 @@ public static Types.StructType fieldStatsFor(Types.NestedField field, int baseFi
baseFieldId + AVG_VALUE_SIZE.offset(),
AVG_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Avg value size of variable-length types (String, Binary)"));
+ "Avg value size in bytes of variable-length types (String, Binary)"));
fields.add(
optional(
baseFieldId + MAX_VALUE_SIZE.offset(),
MAX_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Max value size of variable-length types (String, Binary)"));
+ "Max value size in bytes of variable-length types (String, Binary)"));
Review Comment:
Thanks for taking a look into @nastra, I believe it will be a bit tricky if based on table versions we store different sizes (compressed encoded for v1 - v3 and then v4 uncompressed and un encoded), think of tables migrated from v3 to v4, i think there have been proposals to just have a dedicated metric for this as well like in the doc https://github.com/apache/iceberg/issues/10703, which would be a bit easier to reason about specially for migrated table
For CBO integration if you see engines such as Trino are already having a constant multiplier added to compensate for this (based on TPC-DS) which is just a type check on the column Handle
https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TableStatisticsReader.java#L237-L250
Spark CBO by default is off, but i think where injection point is considered we have estimateStats in the connnector which would also be suff easy to reason.
I believe from integ pov to have a dedicated metric would be very nice, specially for migrated tables v3 -> v4
Please let me know wdyt ?
--
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: Include size unit in avg/max value size fields [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15939:
URL: https://github.com/apache/iceberg/pull/15939#discussion_r3072653544
##########
api/src/main/java/org/apache/iceberg/stats/FieldStatistic.java:
##########
@@ -125,13 +125,13 @@ public static Types.StructType fieldStatsFor(Types.NestedField field, int baseFi
baseFieldId + AVG_VALUE_SIZE.offset(),
AVG_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Avg value size of variable-length types (String, Binary)"));
+ "Avg value size in bytes of variable-length types (String, Binary)"));
fields.add(
optional(
baseFieldId + MAX_VALUE_SIZE.offset(),
MAX_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Max value size of variable-length types (String, Binary)"));
+ "Max value size in bytes of variable-length types (String, Binary)"));
Review Comment:
> I believe they are compressed and encoded size for variable length types, do we wanna add that too there had been past confusions about this
I've been looking more into this and I think it depends on our future calculation whether we use the compressed vs uncompressed sizes from Parquet. I think avg / max value sizes should represent the uncompressed / unencoded size to have better estimates for CBO in Spark/Trino.
In v1-v3 we have the columnSizes metrics, which indeed uses the compressed column size, but for v4 we don't have that metric anymore and we should most likely use `getTotalUncompressedSize()` from `ColumnChunkMetadata` for the calculations of avg / max value sizes.
@singhpk234 does that make sense to you?
Also cc @anoopj
--
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: Include size unit in avg/max value size fields [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15939:
URL: https://github.com/apache/iceberg/pull/15939#discussion_r3067150839
##########
api/src/main/java/org/apache/iceberg/stats/FieldStatistic.java:
##########
@@ -125,13 +125,13 @@ public static Types.StructType fieldStatsFor(Types.NestedField field, int baseFi
baseFieldId + AVG_VALUE_SIZE.offset(),
AVG_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Avg value size of variable-length types (String, Binary)"));
+ "Avg value size in bytes of variable-length types (String, Binary)"));
fields.add(
optional(
baseFieldId + MAX_VALUE_SIZE.offset(),
MAX_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Max value size of variable-length types (String, Binary)"));
+ "Max value size in bytes of variable-length types (String, Binary)"));
Review Comment:
I believe they are compressed and encoded size for variable length types, do we wanna add that too there had been past confusions about this
https://github.com/apache/iceberg/issues/10703
i believe since now parquet supports tracking uncompressed / unencoded size we can support the them also and take this opportunity to clarify our existing metrics, wdyt ?
--
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: Include size unit in avg/max value size fields [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15939:
URL: https://github.com/apache/iceberg/pull/15939#discussion_r3069029517
##########
api/src/main/java/org/apache/iceberg/stats/FieldStatistic.java:
##########
@@ -125,13 +125,13 @@ public static Types.StructType fieldStatsFor(Types.NestedField field, int baseFi
baseFieldId + AVG_VALUE_SIZE.offset(),
AVG_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Avg value size of variable-length types (String, Binary)"));
+ "Avg value size in bytes of variable-length types (String, Binary)"));
fields.add(
optional(
baseFieldId + MAX_VALUE_SIZE.offset(),
MAX_VALUE_SIZE.fieldName(),
Types.IntegerType.get(),
- "Max value size of variable-length types (String, Binary)"));
+ "Max value size in bytes of variable-length types (String, Binary)"));
Review Comment:
we may want to clarify existing metrics in a separate PR. I'll follow up on this as I don't want to mix in the renames with other improvements here. Thanks for bringing this up
--
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