LuciferYang commented on code in PR #55924:
URL: https://github.com/apache/spark/pull/55924#discussion_r3921319232
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetDeltaByteArrayEncodingSuite.scala:
##########
@@ -100,6 +214,117 @@ class ParquetDeltaByteArrayEncodingSuite extends
ParquetCompatibilityTest with S
geoType)
}
+ testGeo("geo interleaves skipBinary with readGeoData (null/skipped rows)") {
geoType =>
+ // A geometry column with null or skipped rows alternates skipBinary (used
for the
+ // skipped rows) with readGeometry/readGeography on the same reader. Both
paths now
+ // share the reusable prevBuf, so the shared prefix carried across a
skipped value must
+ // still be honored by the following read. The values also exceed
prevBuf's initial
+ // 64-byte capacity, exercising the grow-and-preserve branch under
interleaving.
+ assertGeoReadWriteWithSkip(writer, reader, sharedPrefixPolygons(6),
geoType)
Review Comment:
The geo skip-interleave case cannot tell a bug from working code. All six
polygons are 221 bytes and every consecutive `prefixLength` is 197, so
`prevBuf` grows to 221 once and never again: `readGeoData`'s `System.arraycopy`
only ever runs with `prefixLength` 0, and the grow-and-preserve branch named at
:222 is never taken. The common prefix of v0 and v2 is also 197 bytes, so if
`skipBinary` only advanced `in` and stopped writing into `prevBuf`, v2 would
still decode correctly and the test would still pass.
Values of differing lengths, where consecutive values share a longer prefix
than values two apart, would fix it; `longPrefixValues` on the non-geo side
already has that shape. One thing to keep in mind: `corrupt first geo value
with a non-zero prefix fails fast` at :266 relies on a 202-byte shared prefix,
so it must not drop to 0 when the values are reshaped.
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaByteArrayReader.java:
##########
@@ -79,20 +84,31 @@ private void readValues(int total, WritableColumnVector c,
int rowId) {
// value of the page should have an empty prefix, it may not
// because of PARQUET-246.
int prefixLength = prefixLengthVector.getInt(currentRow);
- ByteBuffer suffix = suffixReader.getBytes(currentRow);
- byte[] suffixArray = suffix.array();
- int suffixLength = suffix.limit() - suffix.position();
+ int suffixLength = suffixReader.getSuffixLength(currentRow);
int length = prefixLength + suffixLength;
- // We have to do this to materialize the output
+ // The prefix is shared with the previously decoded value, so it can be
at most as long
+ // as that value. A larger prefixLength means the file is corrupt (e.g.
PARQUET-246 on the
+ // first value of the first page). prevBuf is pre-zeroed and reused, so
without this guard
+ // such input would silently assemble stale/zero prefix bytes instead of
failing.
+ checkPrefixLength(prefixLength);
+
+ // Grow prevBuf if needed, preserving the prefix bytes already in place.
+ if (length > prevBuf.length) {
+ byte[] newBuf = new byte[Math.max(length, prevBuf.length * 2)];
Review Comment:
The suffix length from `getSuffixLength` reaches `new byte[Math.max(length,
prevBuf.length * 2)]` without ever being checked against the bytes actually
left in the page, so the allocation now happens before the read. On master the
same input went through `in.slice(length)`, and `SingleBufferInputStream.slice`
compares `remaining()` and throws `EOFException` with no allocation at all,
surfacing as a plain `ParquetDecodingException`. A corrupt page that used to
give a clear decoding error now drives an allocation of up to 2GB, i.e. an
`OutOfMemoryError`, which being an `Error` is not covered by
`ignoreCorruptFiles` and takes the other tasks in that executor down with it.
`getSuffixLength` is the natural home for the length check: `in` is private
to `VectorizedDeltaLengthByteArrayReader` so the grow branch cannot reach it,
and all three loops already call `getSuffixLength` before growing. At that
point `in` sits exactly at this row's suffix, `available()` is exact, and
anything larger is a corrupt page. A negative `suffixLength` belongs there too;
a negative `prefixLength` belongs in `checkPrefixLength`, which today only
guards the upper bound.
--
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]
