TASK 10: users need explicit sql functions to create geography values#12
TASK 10: users need explicit sql functions to create geography values#12gkalashyan-akv wants to merge 18 commits into
Conversation
b85f434 to
e7d61dd
Compare
| Pattern.compile( | ||
| "\\b(POINT|LINESTRING|POLYGON|MULTIPOINT|MULTILINESTRING|MULTIPOLYGON|GEOMETRYCOLLECTION)\\s+(ZM|Z|M)\\b", | ||
| Pattern.CASE_INSENSITIVE); | ||
| private static final WKTReader WKT_READER = createWktReader(); |
There was a problem hiding this comment.
Could we avoid sharing WKBReader/WKBWriter as static singletons here? JTS documents these classes as reusable but not thread-safe, and scalar functions may be evaluated concurrently by different tasks/threads in the same JVM. A per-call instance or ThreadLocal would make the constructor functions safe under parallel execution. We can apply the same pattern to WKTReader for consistency.
| throw new TableRuntimeException("Invalid GEOGRAPHY WKB.", e); | ||
| } | ||
|
|
||
| validateGeometry(readGeometry(geography)); |
There was a problem hiding this comment.
Could we make the WKB validation boundary stricter here? fromWkb() validates by reading through JTS but returns the original WKB bytes. JTS WKBReader repairs some structurally invalid inputs such as LineStrings/LinearRings with too few points or non-closed rings, so those inputs may validate successfully while we still store the unrepaired raw WKB. Since ST_GEOGFROMWKB is the constructor boundary, I think we should either add explicit strict checks for those WKB invariants before returning the raw bytes, or add tests that confirm such inputs are rejected.
fromWkb() validates by reading with JTS, but then returns the original geography bytes. JTS WKBReader is lenient: its source says it repairs structurally invalid LineStrings/LinearRings, e.g. too few points or non-closed rings. That means invalid WKB can pass validation while Flink stores the original invalid bytes.
| "Invalid GEOGRAPHY coordinates. Only 2D coordinates are supported."); | ||
| } | ||
|
|
||
| final double longitude = sequence.getX(i); |
There was a problem hiding this comment.
Could we reject non-finite coordinates before the CRS84 range checks? JTS WKT supports NaN, and Java comparisons with NaN return false, so values such as POINT (NaN 0) can bypass the current longitude/latitude checks. We should probably allow explicit empty geometries, but reject NaN/infinite ordinates in non-empty coordinate sequences.
The CRS84 range checks don’t reject NaN. JTS WKTReader explicitly supports NaN numbers, and comparisons like longitude < -180D / longitude > 180D are false for NaN, so POINT (NaN 0)-style input can slip through. POINT EMPTY is fine, but non-empty non-finite coordinates should not become valid GEOGRAPHY.
3711c14 to
83088b1
Compare
65b599b to
0bf4ad7
Compare
0bf4ad7 to
9c4b37c
Compare
What is the purpose of the change
(For example: This pull request makes task deployment go through the blob server, rather than through RPC. That way we avoid re-transferring them on each deployment (during recovery).)
Brief change log
(for example:)
Verifying this change
Please make sure both new and modified tests in this PR follow the conventions for tests defined in our code quality guide.
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation
Was generative AI tooling used to co-author this PR?