stevenzwu commented on code in PR #18394:
URL: https://github.com/apache/iceberg/pull/18394#discussion_r4192226353
##########
core/src/main/java/org/apache/iceberg/FieldStatsStruct.java:
##########
@@ -231,6 +232,9 @@ private static int[] posToOffset(Types.StructType struct) {
private static Object copyBound(Object bound) {
if (bound instanceof byte[] bytes) {
return copyOf(bytes);
+ } else if (bound instanceof GeospatialBound geo) {
+ // readers reuse one bound instance
+ return geo.copy();
} else if (bound instanceof StructLike struct) {
return StructLikeUtil.copy(struct);
Review Comment:
We kept this StructLike copy because geo bounds are not always
GeospatialBound.
TestFieldStatsStruct.geoSerialization still builds lower and upper bounds as
PartitionData. FieldStats#copy needs StructLikeUtil.copy so the copy is not the
same instance. Dropping this branch fails the geometry and geography
FieldStats#copy cases on isNotSameAs. The other serializers in that test still
pass.
There is also a read path that skips GeospatialBound registration. If
tableSchema.findField returns null, or the column is no longer geometry or
geography, V4ManifestReader does not call setCustomType on the bound fields.
Those bounds are GenericRecord. With reuseContainers(), copyResolved would
alias the same instance across manifest entries without a deep copy.
We are exploring removing this branch and adjusting the unit tests to use
GeospatialBound instead of PartitionData.
--
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]
