iemejia commented on PR #55924:
URL: https://github.com/apache/spark/pull/55924#issuecomment-5511255801
Thanks @LuciferYang, all three addressed in 1cd386441c0.
1. **`readGeoData` allocation.** Rather than just scoping the description, I
removed the per-row `new byte[length]`: `readGeoData` now assembles the WKB
into the same reusable `prevBuf` and shares the grow-and-preserve protocol with
`readBinary`/`skipBinary`. To avoid trailing stale bytes riding along when
`prevBuf` capacity exceeds the value, `convert` now takes an offset/length and
passes `[0, length)` down. The overload is plumbed through
`WKBConverterStrategy.convert` -> `STUtils.stGeom/stGeogFromWKB` ->
`Geometry/Geography.fromWkb` -> `WkbReader.read` (which now wraps just the
`[offset, offset+length)` sub-range). The existing 2-arg `convert` stays as a
`default` delegate, so the other call sites (plain reader, updater factory,
length-byte-array reader) are untouched. Net effect: `readGeoData` is now
allocation-free except for the physical output value itself, which is the
decoded result and unavoidable. The description is updated to reflect this.
2. **Retire `getBytes(int)`.** Done -- removed from
`VectorizedDeltaLengthByteArrayReader`. All three call sites use
`getSuffixLength`/`getSuffixInto`, and since it is package-private and never
escapes the parquet package there is no compatibility concern.
3. **Geo test gaps.** Added to `ParquetDeltaByteArrayEncodingSuite` (each
runs across `GeometryType(0)`, `GeometryType(4326)`, `GeographyType(4326)`):
- `geo interleaves skipBinary with readGeoData (null/skipped rows)` --
long (>64B) shared-prefix polygons, reading even rows and skipping odd ones so
a read must honor a prefix carried across a skipped value. With the change both
paths use the same `prevBuf`, so this exercises that consistency plus the grow
branch under interleaving.
- `geo setPreviousReader recovers a long value across pages
(PARQUET-246)` -- a recovered previous value >64B exercises the grow +
deep-copy in `setPreviousReader` before `readGeoData` reuses it.
- Unit tests `testStGeom/GeogFromWKBWithOffsetAndLength` verify the
offset/length parse ignores surrounding filler bytes.
PTAL, thanks!
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
