dalelane commented on PR #28740: URL: https://github.com/apache/flink/pull/28740#issuecomment-5304353878
@davidradl https://github.com/apache/flink/pull/28740#pullrequestreview-4937703586 > The one I'd block merge on > (int) cast truncation in BinaryGeographyData.readUnsignedInt() — the validator traverses the WKB tree with a long cursor, but every call to BinarySegmentUtils.getByte(segments, (int) offset) silently truncates the offset. For any geometry whose internal offset exceeds Integer.MAX_VALUE in a native memory segment, this either reads the wrong byte or throws an out-of-bounds exception. Either document a hard 2 GB limit and enforce it in checkRange, or pass the long through to a 64-bit accessor. I think the use of a `long` is intentional - it's about protecting against arithmetic overflow if the data was corrupted somehow. WKB data contains counts. A LineString declares how many points it has, and we scale that up to work out how many bytes are needed. If we did use an int for that, we really would run the risk of an overflow. If you imagine malformed data claiming that it had 4 billion points (which is about as high as it could claim as an unsigned 32-bit number) in a 100-byte payload: 1. `numPoints` = 4,000,000,000 (read as a long) 2. 4,000,000,000 points × 16 = 64,000,000,000 bytes (again, computed as a long, so theres no wraparound) 3. `requireBytes` compares that against the 100 bytes actually available, and can throw the exception 4. `getByte` is never called, so no cast to an int never happens The point is that the huge number gets rejected while it's still a long. Only values confirmed to fit inside the real payload will reach the cast. I don't think it's reasonable to describe this as silently truncating, as each cast is preceded by a bounds check. At any rate, offset points into a Flink binary row and Flink's binary format is int-addressed (the offset parameter in getByte is an int). All the binary types have the same 2gb ceiling, so I don't think a 64-bit accessor is an available option here. @davidchaava - maybe it's worth adding a comment to help the next reader not tripping over this though? . > isFlinkExtensionType() in FlinkTypeFactory is a growing instanceof chain (RAW, BITMAP, now GEOGRAPHY) with no marker interface — a guaranteed maintenance hazard for every future extension type. It's only three items, so this wouldn't worry me overly, but perhaps the more interesting aspect is that this would change how `leastRestrictive` is handled for RAW and BITMAP. Before this change, they would fall through to Calcite's `super.leastRestrictive`. But with this change, they'll go to `LogicalTypeMerging.findCommonType` instead. @davidchaava - is that intentional? . > skipTypedGeometryCollection() receives a byteOrder param it never uses — dead parameter that misleads readers. It does use it. In the first line, it calls `readUnsignedInt` with it, which uses it to switch on how to use `BinarySegmentUtils.getByte` . > createInstance()'s raw byte literal for the empty GEOMETRYCOLLECTION is fragile — no test validates it; compare to BitmapSerializer which builds its default via the public API. `testCreateInstanceReturnsValidGeographyData` in `GeographyTypeSerializerTest` has a test that validates it And because it's included in `getTestData()`, it'll have the entire SerializerTestBase suite run against it. (Feels like it's low risk at any rate, because if the literal was wrong, it'd throw an exception the first time it was used) -- 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]
