zhangfengcdt commented on code in PR #3222:
URL: https://github.com/apache/parquet-java/pull/3222#discussion_r2102716572
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geospatial/BoundingBox.java:
##########
@@ -218,16 +233,27 @@ public void merge(BoundingBox other) {
return;
}
- this.xMin = Math.min(this.xMin, other.xMin);
- this.xMax = Math.max(this.xMax, other.xMax);
+ // We don't yet support merging wraparound bounds.
+ // Rather than throw, we mark the X bounds as invalid.
+ if (isXWraparound() || other.isXWraparound()) {
+ // Mark X dimension as invalid by setting to NaN
+ xMin = Double.NaN;
+ xMax = Double.NaN;
Review Comment:
Thanks for the review @wgtmac !
For the reseting the bbox, my read from the C++ implementation below is that
this wrap_x only impacts the x dimension, and it does not reset the whole bbox.
This way, other dimensions are still namely "valid". I am good if we want to
change this in both implementations. CC: @paleolimbot for opinion on this.
```
// We don't yet support updating wraparound bounds. Rather than throw,
// we just mark the X bounds as invalid such that they are not used.
auto other_bounds = other.bounder_.Bounds();
if (is_wraparound_x() || other.is_wraparound_x()) {
other_bounds.min[0] = kNaN;
other_bounds.max[0] = kNaN;
}
```
The valid flag is set below by calling "valid = isXYValid();", so I think we
are good here.
--
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]
