davidchaava commented on PR #28740: URL: https://github.com/apache/flink/pull/28740#issuecomment-5327739353
@davidradl @dalelane thanks for the detailed review. We went through the findings and updated the PR accordingly. > The validator traverses the WKB tree with a long cursor, but the accessor truncates the offset to int. Addressed. We added checked address conversion and range validation in `BinaryGeographyData`, together with tests covering large unsigned values and invalid ranges. > `writeGeography()` ... always allocates a new heap array. Addressed. `AbstractBinaryWriter` now uses a zero-copy path for `BinaryGeographyData` and keeps the existing byte-array fallback for other implementations. > `getGeography()` ... calls `GeographyData.fromBytes()` on every row access. Addressed. Columnar GEOGRAPHY access now uses lazy trusted binary views. > WKB subtype constants ... need clarification. Addressed. The supported 2D ISO WKB subtype contract is now documented, including the distinction from EWKB and extended dimensional encodings. > `createInstance()`'s raw byte literal for the empty `GEOMETRYCOLLECTION` is fragile. Addressed. We added a test confirming that the default instance is a valid `GEOMETRYCOLLECTION`. > The unrelated async test fix ... should be a separate PR. The unrelated change has been removed from this PR. We also expanded the GEOGRAPHY documentation with references for CRS, SRID, OGC:CRS84, ISO WKB, and EWKB. The SQL constructors, accessors, and spatial functions are being handled separately in follow-up PR #28788. The PR should now be ready for another review round. Thanks again for the thorough feedback. -- 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]
