Commit c50c662
authored
[PART-1] Figure and Shape Parser support (#4681)
Fix geography validation bugs, replace ad-hoc CLR binary metadata parsing with spec-compliant implementation, and fix polygon Valid flag in properties byte.
**Currently**, Babelfish has the following issues in `spatialtypes.c`:
1. **Geography accepts out-of-range longitude values.** `geography::Point(45, 20000, 4326)` succeeds silently. T-SQL enforces longitude bounds of −15069 to +15069 degrees and rejects this input.
2. **Geometry/geography accepts Infinity coordinates.** Binary data containing `±Infinity` coordinate values passes all validation because only `isnan()` is checked, not `isinf()`. T-SQL rejects both NaN and Infinity.
3. **CLR binary metadata (Figure/Shape arrays) is parsed incorrectly.** The old `check_geom_end_metadata()` reads 4 bytes at a time across 5-byte figure entry boundaries. It works only because the attribute byte happens to produce the expected uint32 value when the following offset bytes are zeros. No bounds checking exists on metadata reads.
4. **Polygon properties byte missing Valid flag.** Babelfish outputs `0x00` for valid 2D polygons while T-SQL outputs `0x04` (V flag set). Linestrings already handled valid/invalid variants correctly.
5. **`set_dimension_flag()` prematurely guesses geometry type.** Properties byte `0x04` was hardcoded as `LINE_TYPE`, creating a latent bug where valid polygons from T-SQL would be misidentified.
**With this change:**
1. New `validate_longitude_range()` and `validate_not_inf_nan()` functions added and integrated into all validation paths (text input, binary input, direct point creation). Geography objects with longitude outside ±15069° are now rejected.
2. `check_nan_coordinates()` now checks both `isnan()` and `isinf()`. All geography validation paths also check for infinity.
3. New `parse_figures_and_shapes()` replaces `check_geom_end_metadata()`. Reads figure entries as 1+4 bytes and shape entries as 4+4+1 bytes per the T-SQL CLR spec (sections 2.1.3–2.1.4). Includes `CHECK_METADATA_BOUNDS` macro for buffer safety. Geometry type determined from Shape type byte — the single authoritative source.
4. Added `VALID_POLYGON_2D` through `VALID_POLYGON_3DM` constants. `determine_geom_dimensions()` now uses `is_valid` for polygons, matching the existing linestring pattern.
5. `set_dimension_flag()` no longer guesses `LINE_TYPE` for `0x04`. Falls through to `COMPLEX_GEOM_2D`, letting `parse_figures_and_shapes()` determine the actual type from Shape metadata.
6. `geography_from_bytea()` returns NULL for SRID = −1 input instead of crashing on length validation.
### Issues Resolved
- Geography silently accepts longitude values outside ±15069° range
- Geometry/geography silently accepts ±Infinity coordinates
- CLR Figure metadata parsed by reading 4 bytes across 5-byte entry boundaries (works by coincidence)
- CLR Shape metadata not parsed at all (geometry type guessed from byte patterns)
- Polygon properties byte missing Valid flag (byte 5 mismatch with T-SQL)
- `set_dimension_flag()` hardcodes `LINE_TYPE` for properties byte `0x04` (latent bug for valid polygons)
- NULL geography (SRID = −1) causes crash instead of returning NULL
### Test Scenarios Covered ###
* **Use case based -**
- Point: 2D, 3DM, Empty (geometry)
- LineString: 2-point, multi-point, Empty (geometry)
- Polygon: invalid 2D, invalid 3D, invalid multi-ring, valid 2D, valid 3D, valid 3DM, self-intersecting, Empty (geometry)
- Geography: Point, LineString, single-ring Polygon
- Round-trip: geometry → varbinary → geometry for polygon
* **Boundary conditions -**
- `geography::Point(45, 15069, 4326)` — longitude at positive boundary, must succeed
- `geography::Point(45, -15069, 4326)` — longitude at negative boundary, must succeed
- `geography::Point(90, 0, 4326)` — latitude at boundary, must succeed
* **Arbitrary inputs -**
- 3-ring polygon (exterior + 2 interior rings)
- Valid polygon (proper square) to verify V flag in properties byte
- Self-intersecting polygon (invalid geometry)
* **Negative test cases -**
- `geography::Point(45, 20000, 4326)` — longitude out of range, must error
- `geography::Point(45, -20000, 4326)` — negative longitude out of range, must error
- `geography::STGeomFromText('LINESTRING(0 0, 20000 45, 1 1)', 4326)` — longitude in linestring, must error
- `geography::Point(91, 0, 4326)` — latitude out of range, must error
- `CAST(0x00000000010C000000000000F07F0000000000004940 AS geometry)` — Infinity coordinate, must error
- `CAST(0x00000000010C000000000000F87F0000000000004940 AS geometry)` — NaN coordinate, must error
- `CAST(0xFFFFFFFF AS geography)` — SRID = −1, must return NULL
* **Minor version upgrade tests -**
N/A — no catalog or protocol changes
* **Major version upgrade tests -**
N/A — no catalog or protocol changes
* **Performance tests -**
N/A — validation adds negligible overhead (one extra isinf() call per coordinate, one memcpy for longitude extraction)
* **Tooling impact -**
N/A — no changes to tooling
* **Client tests -**
N/A — no client protocol changes
Task: BABEL-6377
Signed-off-by: Gopal Verma <gopalgv@amazon.com>1 parent 193c03d commit c50c662
File tree
23 files changed
+16856
-4816
lines changed- contrib/babelfishpg_common/src
- test
- JDBC
- expected
- input/datatypes
- upgrade
- 17_10
- 17_9
- 18_3
- dotnet/ExpectedOutput
- odbc/psqlodbc/test
23 files changed
+16856
-4816
lines changedLarge diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
217 | 217 | | |
218 | 218 | | |
219 | 219 | | |
220 | | - | |
| 220 | + | |
221 | 221 | | |
222 | 222 | | |
223 | 223 | | |
224 | 224 | | |
225 | 225 | | |
226 | 226 | | |
227 | | - | |
| 227 | + | |
228 | 228 | | |
229 | 229 | | |
230 | 230 | | |
| |||
237 | 237 | | |
238 | 238 | | |
239 | 239 | | |
240 | | - | |
241 | | - | |
242 | | - | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
243 | 244 | | |
244 | 245 | | |
245 | 246 | | |
| |||
251 | 252 | | |
252 | 253 | | |
253 | 254 | | |
254 | | - | |
255 | | - | |
256 | | - | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
257 | 259 | | |
258 | 260 | | |
259 | 261 | | |
| |||
Lines changed: 143 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
0 commit comments