Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.6k
MINOR: Update BoundingBox for Empty and Antimeridian Handling#3222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -18,6 +18,7 @@ | ||||||||||||
| */ | ||||||||||||
| package org.apache.parquet.column.statistics.geospatial; | ||||||||||||
| import org.apache.parquet.ShouldNeverHappenException; | ||||||||||||
| import org.locationtech.jts.geom.Coordinate; | ||||||||||||
| import org.locationtech.jts.geom.Envelope; | ||||||||||||
| import org.locationtech.jts.geom.Geometry; | ||||||||||||
| @@ -169,7 +170,7 @@ public boolean isXYEmpty() { | ||||||||||||
| * @return true if the X dimension is empty, false otherwise. | ||||||||||||
| */ | ||||||||||||
| public boolean isXEmpty() { | ||||||||||||
| return Double.isInfinite(xMin) && Double.isInfinite(xMax); | ||||||||||||
| return Double.isInfinite(xMin - xMax); | ||||||||||||
| } | ||||||||||||
| /** | ||||||||||||
| @@ -178,7 +179,7 @@ public boolean isXEmpty() { | ||||||||||||
| * @return true if the Y dimension is empty, false otherwise. | ||||||||||||
| */ | ||||||||||||
| public boolean isYEmpty() { | ||||||||||||
| return Double.isInfinite(yMin) && Double.isInfinite(yMax); | ||||||||||||
| return Double.isInfinite(yMin - yMax); | ||||||||||||
| } | ||||||||||||
| /** | ||||||||||||
| @@ -187,7 +188,7 @@ public boolean isYEmpty() { | ||||||||||||
| * @return true if the Z dimension is empty, false otherwise. | ||||||||||||
| */ | ||||||||||||
| public boolean isZEmpty() { | ||||||||||||
| return Double.isInfinite(zMin) && Double.isInfinite(zMax); | ||||||||||||
| return Double.isInfinite(zMin - zMax); | ||||||||||||
| } | ||||||||||||
| /** | ||||||||||||
| @@ -196,14 +197,28 @@ public boolean isZEmpty() { | ||||||||||||
| * @return true if the M dimension is empty, false otherwise. | ||||||||||||
| */ | ||||||||||||
| public boolean isMEmpty() { | ||||||||||||
| return Double.isInfinite(mMin) && Double.isInfinite(mMax); | ||||||||||||
| return Double.isInfinite(mMin - mMax); | ||||||||||||
| } | ||||||||||||
| /** | ||||||||||||
| * Checks if the X dimension of this bounding box wraps around the antimeridian. | ||||||||||||
| * This occurs when the minimum X value is greater than the maximum X value, | ||||||||||||
| * which is allowed by the Parquet specification for geometries that cross the antimeridian. | ||||||||||||
| * | ||||||||||||
| * @return true if the X dimension wraps around, false otherwise. | ||||||||||||
| */ | ||||||||||||
| public boolean isXWraparound() { | ||||||||||||
| return isWraparound(xMin, xMax); | ||||||||||||
| } | ||||||||||||
| /** | ||||||||||||
| * Expands this bounding box to include the bounds of another box. | ||||||||||||
| * After merging, this bounding box will contain both its original extent | ||||||||||||
| * and the extent of the other bounding box. | ||||||||||||
| * | ||||||||||||
| * If either this bounding box or the other has wraparound X coordinates, | ||||||||||||
| * the X dimension will be marked as invalid (set to NaN) in the result. | ||||||||||||
| * | ||||||||||||
| * @param other the other BoundingBox whose bounds will be merged into this one | ||||||||||||
| */ | ||||||||||||
| public void merge(BoundingBox other) { | ||||||||||||
| @@ -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. | ||||||||||||
wgtmac marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||||||||||||
| if (isXWraparound() || other.isXWraparound()) { | ||||||||||||
| // Mark X dimension as invalid by setting to NaN | ||||||||||||
| xMin = Double.NaN; | ||||||||||||
| xMax = Double.NaN; | ||||||||||||
Comment on lines
+239
to
+241
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Can we simply do this? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. The valid flag is set below by calling "valid = isXYValid();", so I think we are good here. Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the current code here...eventually the inside of that
| ||||||||||||
| } else { | ||||||||||||
| // Normal case - merge X bounds | ||||||||||||
| this.xMin = Math.min(this.xMin, other.xMin); | ||||||||||||
| this.xMax = Math.max(this.xMax, other.xMax); | ||||||||||||
| } | ||||||||||||
| // Always merge Y, Z, and M bounds | ||||||||||||
| this.yMin = Math.min(this.yMin, other.yMin); | ||||||||||||
| this.yMax = Math.max(this.yMax, other.yMax); | ||||||||||||
| this.zMin = Math.min(this.zMin, other.zMin); | ||||||||||||
| this.zMax = Math.max(this.zMax, other.zMax); | ||||||||||||
| this.mMin = Math.min(this.mMin, other.mMin); | ||||||||||||
| this.mMax = Math.max(this.mMax, other.mMax); | ||||||||||||
| // Update the validity of this bounding box based on the other bounding box | ||||||||||||
| // Update the validity of this bounding box | ||||||||||||
| valid = isXYValid(); | ||||||||||||
| } | ||||||||||||
| @@ -272,10 +298,28 @@ public void update(Geometry geometry) { | ||||||||||||
| * - X bounds are only updated if both minX and maxX are not NaN | ||||||||||||
| * - Y bounds are only updated if both minY and maxY are not NaN | ||||||||||||
| * | ||||||||||||
| * This allows partial updates while preserving valid dimensions. | ||||||||||||
| * Note: JTS (Java Topology Suite) does not natively support wraparound envelopes | ||||||||||||
| * or geometries that cross the antimeridian (±180° longitude). It operates strictly | ||||||||||||
| * in a 2D Cartesian coordinate space and doesn't account for the Earth's spherical | ||||||||||||
| * nature or longitudinal wrapping. | ||||||||||||
| * | ||||||||||||
| * When JTS encounters a geometry that crosses the antimeridian, it will represent | ||||||||||||
| * it with an envelope spanning from the westernmost to easternmost points, often | ||||||||||||
| * covering most of the Earth's longitude range (e.g., minX=-180, maxX=180). | ||||||||||||
| * | ||||||||||||
| * The wraparound check below is defensive but should never be triggered with standard | ||||||||||||
| * JTS geometry operations, as JTS will never produce an envelope with minX > maxX. | ||||||||||||
| * | ||||||||||||
| * @throws ShouldNeverHappenException if the update creates an X wraparound condition | ||||||||||||
| */ | ||||||||||||
| private void updateBounds(double minX, double maxX, double minY, double maxY) { | ||||||||||||
| if (!Double.isNaN(minX) && !Double.isNaN(maxX)) { | ||||||||||||
| // Check if the update would create a wraparound condition | ||||||||||||
| // This should never happen with standard JTS geometry operations | ||||||||||||
| if (isWraparound(minX, maxX) || isWraparound(xMin, xMax)) { | ||||||||||||
| throw new ShouldNeverHappenException("Wraparound bounding boxes are not yet supported"); | ||||||||||||
| } | ||||||||||||
| xMin = Math.min(xMin, minX); | ||||||||||||
| xMax = Math.max(xMax, maxX); | ||||||||||||
| } | ||||||||||||
| @@ -302,6 +346,16 @@ public void reset() { | ||||||||||||
| valid = true; | ||||||||||||
| } | ||||||||||||
| /** | ||||||||||||
| * The Parquet specification allows X bounds to be "wraparound" to allow for | ||||||||||||
| * more compact bounding boxes when a geometry happens to include components | ||||||||||||
| * on both sides of the antimeridian (e.g., the nation of Fiji). This function | ||||||||||||
| * checks for that case. | ||||||||||||
| */ | ||||||||||||
| public static boolean isWraparound(double xmin, double xmax) { | ||||||||||||
| return !Double.isInfinite(xmin - xmax) && xmin > xmax; | ||||||||||||
| } | ||||||||||||
| /** | ||||||||||||
| * Creates a copy of the current bounding box. | ||||||||||||
| * | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -530,34 +530,34 @@ public void testLineStringWithPartialNaNCoordinates() { | ||
| /** | ||
| * Tests the end-to-end case for updating and merging bounding boxes with mixed valid and NaN coordinates. | ||
| * | ||
| * <p> | ||
| * Scenario - Parquet file with multiple row groups: | ||
| * file-level bbox: [1, 9, 100, 900] | ||
| * | ||
| * <p> | ||
| * Row group 1: [1, 2, 100, 100] | ||
| * - POINT (1, 100) | ||
| * - POINT (2, NaN) | ||
| * | ||
| * <p> | ||
| * Row group 2: [3, 3, 300, 300] | ||
| * - POINT (3, 300) | ||
| * - POINT (NaN, NaN) | ||
| * | ||
| * <p> | ||
| * Row group 3: no valid bbox | ||
| * - POINT (5, NaN) | ||
| * - POINT (6, NaN) | ||
| * | ||
| * <p> | ||
| * Row group 4: [7, 8, 700, 800] | ||
| * - POINT (7, 700) | ||
| * - POINT (8, 800) | ||
| * | ||
| * <p> | ||
| * Row group 5: no valid bbox | ||
| * - POINT (NaN, NaN) | ||
| * - POINT (NaN, NaN) | ||
| * | ||
| * <p> | ||
| * Row group 6: [9, 9, 900, 900] | ||
| * - POINT (9, 900) | ||
| * - LINESTRING EMPTY | ||
| * | ||
| * <p> | ||
| * The test verifies that: | ||
| * 1. Individual row group bounding boxes correctly handle NaN coordinates | ||
| * 2. The merge operation correctly combines valid bounding boxes and ignores invalid ones | ||
| @@ -681,4 +681,146 @@ public void testMergingRowGroupBoundingBoxes() { | ||
| Assert.assertEquals(900.0, reverseMergeBox.getYMax(), 0.0); | ||
| Assert.assertTrue(reverseMergeBox.isValid()); | ||
| } | ||
| @Test | ||
| public void testIsXValidAndIsYValid() { | ||
| // Test with valid X and Y | ||
| BoundingBox validBox = new BoundingBox(1, 2, 3, 4, 5, 6, 7, 8); | ||
| Assert.assertTrue(validBox.isXValid()); | ||
| Assert.assertTrue(validBox.isYValid()); | ||
zhangfengcdt marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| Assert.assertTrue(validBox.isXYValid()); | ||
| Assert.assertTrue(validBox.isZValid()); | ||
| Assert.assertTrue(validBox.isMValid()); | ||
| // Test with invalid X (NaN) | ||
| BoundingBox invalidXBox = new BoundingBox(Double.NaN, 2, 3, 4, 5, 6, 7, 8); | ||
| Assert.assertFalse(invalidXBox.isXValid()); | ||
| Assert.assertTrue(invalidXBox.isYValid()); | ||
| Assert.assertFalse(invalidXBox.isXYValid()); | ||
| Assert.assertTrue(invalidXBox.isZValid()); | ||
| Assert.assertTrue(invalidXBox.isMValid()); | ||
| // Test with invalid Y (NaN) | ||
| BoundingBox invalidYBox = new BoundingBox(1, 2, Double.NaN, 4, 5, 6, 7, 8); | ||
| Assert.assertTrue(invalidYBox.isXValid()); | ||
| Assert.assertFalse(invalidYBox.isYValid()); | ||
| Assert.assertFalse(invalidXBox.isXYValid()); | ||
| Assert.assertTrue(invalidXBox.isZValid()); | ||
| Assert.assertTrue(invalidXBox.isMValid()); | ||
| // Test with both X and Y invalid | ||
| BoundingBox invalidXYBox = new BoundingBox(Double.NaN, Double.NaN, Double.NaN, Double.NaN, 5, 6, 7, 8); | ||
| Assert.assertFalse(invalidXYBox.isXValid()); | ||
| Assert.assertFalse(invalidXYBox.isYValid()); | ||
| Assert.assertFalse(invalidXYBox.isXYValid()); | ||
| Assert.assertTrue(invalidXBox.isZValid()); | ||
| Assert.assertTrue(invalidXBox.isMValid()); | ||
| } | ||
| @Test | ||
| public void testIsXEmptyAndIsYEmpty() { | ||
| // Empty bounding box (initial state) | ||
| BoundingBox emptyBox = new BoundingBox(); | ||
| Assert.assertTrue(emptyBox.isXEmpty()); | ||
| Assert.assertTrue(emptyBox.isYEmpty()); | ||
| Assert.assertTrue(emptyBox.isXYEmpty()); | ||
| // Non-empty box | ||
| BoundingBox nonEmptyBox = new BoundingBox(1, 2, 3, 4, 5, 6, 7, 8); | ||
| Assert.assertFalse(nonEmptyBox.isXEmpty()); | ||
| Assert.assertFalse(nonEmptyBox.isYEmpty()); | ||
| Assert.assertFalse(nonEmptyBox.isXYEmpty()); | ||
| // Box with empty X dimension only | ||
| GeometryFactory gf = new GeometryFactory(); | ||
| BoundingBox emptyXBox = new BoundingBox(); | ||
| // Only update Y dimension | ||
| emptyXBox.update(gf.createPoint(new Coordinate(Double.NaN, 5))); | ||
| Assert.assertTrue(emptyXBox.isXEmpty()); | ||
| Assert.assertFalse(emptyXBox.isYEmpty()); | ||
| Assert.assertTrue(emptyXBox.isXYEmpty()); | ||
| // Box with empty Y dimension only | ||
| BoundingBox emptyYBox = new BoundingBox(); | ||
| // Only update X dimension | ||
| emptyYBox.update(gf.createPoint(new Coordinate(10, Double.NaN))); | ||
| Assert.assertFalse(emptyYBox.isXEmpty()); | ||
| Assert.assertTrue(emptyYBox.isYEmpty()); | ||
| Assert.assertTrue(emptyYBox.isXYEmpty()); | ||
| } | ||
| @Test | ||
| public void testIsXWraparound() { | ||
| // Normal bounding box (no wraparound) | ||
| BoundingBox normalBox = new BoundingBox(1, 2, 3, 4, 5, 6, 7, 8); | ||
| Assert.assertFalse(normalBox.isXWraparound()); | ||
| // Wraparound box (xMin > xMax) | ||
| BoundingBox wraparoundBox = new BoundingBox(170, 20, 10, 20, 0, 0, 0, 0); | ||
| Assert.assertTrue(wraparoundBox.isXWraparound()); | ||
| // Edge case: equal bounds | ||
| BoundingBox equalBoundsBox = new BoundingBox(10, 10, 20, 20, 0, 0, 0, 0); | ||
| Assert.assertFalse(equalBoundsBox.isXWraparound()); | ||
| // Test static method directly | ||
| Assert.assertTrue(BoundingBox.isWraparound(180, -180)); | ||
| Assert.assertFalse(BoundingBox.isWraparound(-180, 180)); | ||
zhangfengcdt marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| // Test with infinity values | ||
| Assert.assertFalse(BoundingBox.isWraparound(Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY)); | ||
| Assert.assertFalse(BoundingBox.isWraparound(Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY)); | ||
| Assert.assertFalse(BoundingBox.isWraparound(Double.POSITIVE_INFINITY, Double.POSITIVE_INFINITY)); | ||
| Assert.assertFalse(BoundingBox.isWraparound(Double.NEGATIVE_INFINITY, Double.NEGATIVE_INFINITY)); | ||
| // Check edge cases | ||
| Assert.assertFalse(BoundingBox.isWraparound(0.0, Double.POSITIVE_INFINITY)); | ||
| Assert.assertFalse(BoundingBox.isWraparound(Double.NEGATIVE_INFINITY, 0.0)); | ||
| } | ||
| @Test | ||
| public void testWraparoundHandlingInMerge() { | ||
| // Test with two normal boxes | ||
| BoundingBox box1 = new BoundingBox(10, 20, 10, 20, 0, 0, 0, 0); | ||
| BoundingBox box2 = new BoundingBox(15, 25, 15, 25, 0, 0, 0, 0); | ||
| box1.merge(box2); | ||
| Assert.assertTrue(box1.isValid()); | ||
| Assert.assertEquals(10.0, box1.getXMin(), 0.0); | ||
| Assert.assertEquals(25.0, box1.getXMax(), 0.0); | ||
| // Test with one wraparound box | ||
| BoundingBox normalBox = new BoundingBox(0, 10, 0, 10, 0, 0, 0, 0); | ||
| BoundingBox wraparoundBox = new BoundingBox(170, -170, 5, 15, 0, 0, 0, 0); | ||
| normalBox.merge(wraparoundBox); | ||
zhangfengcdt marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| Assert.assertFalse(normalBox.isValid()); | ||
| Assert.assertTrue(Double.isNaN(normalBox.getXMin())); | ||
| Assert.assertTrue(Double.isNaN(normalBox.getXMax())); | ||
| Assert.assertEquals(0.0, normalBox.getYMin(), 0.0); | ||
| Assert.assertEquals(15.0, normalBox.getYMax(), 0.0); | ||
| } | ||
| @Test | ||
| public void testWraparoundBoxMergingNormalBox() { | ||
| // Create a normal bounding box | ||
| BoundingBox normalBox = new BoundingBox(0, 10, 0, 10, 0, 0, 0, 0); | ||
| // Create a wraparound bounding box (xMin > xMax) | ||
| BoundingBox wraparoundBox = new BoundingBox(170, -170, 5, 15, 0, 0, 0, 0); | ||
| // Merge the normal box into the wraparound box | ||
| wraparoundBox.merge(normalBox); | ||
| // After merging, X dimension should be marked as invalid (NaN) | ||
| // because we don't support merging wraparound bounds | ||
| Assert.assertFalse(wraparoundBox.isValid()); | ||
| Assert.assertTrue(Double.isNaN(wraparoundBox.getXMin())); | ||
| Assert.assertTrue(Double.isNaN(wraparoundBox.getXMax())); | ||
| // Y dimension should be properly merged | ||
| Assert.assertEquals(0.0, wraparoundBox.getYMin(), 0.0); | ||
| Assert.assertEquals(15.0, wraparoundBox.getYMax(), 0.0); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.