PDGGK opened a new pull request, #9275: URL: https://github.com/apache/paimon/pull/9275
### Purpose `VectorizedDeltaByteArrayReader` keeps the previous decoded value as a **view over the output vector's buffer**: ```java c.putByteArray(rowId + i, bytes, offset, length); BytesColumnVector.Bytes b = c.getBytes(rowId + i); previous = ByteBuffer.wrap(b.data, b.offset, b.len); // :96 ``` `HeapBytesVector.getBytes` returns `new Bytes(buffer, start[i], length[i])` (`:153-155`) — `b.data` **is** the vector's live buffer. And `HeapBytesVector.reset()` zeroes it: ```java // We don't reset buffer to avoid unnecessary copy. Arrays.fill(buffer, (byte) 0); // HeapBytesVector:75-76 ``` The comment says the buffer is left alone; the line under it fills the whole thing with zeros. `VectorizedParquetRecordReader:201` resets the vector between batches. So when a DELTA_BYTE_ARRAY page spans more than one batch — the normal case, since a page usually holds far more values than `read.batch-size` — `previous` points at a zeroed region, and every prefix copied from it is NUL bytes. ### Measured Eight values sharing a 14-character prefix, read as two batches of four with a `reset()` between, exactly as the record reader does: ``` batch2[0] expected=shared-prefix-0004 actual=<NUL>×17 + "4" batch2[1] expected=shared-prefix-0005 actual=<NUL>×17 + "5" batch2[2] expected=shared-prefix-0006 actual=<NUL>×17 + "6" batch2[3] expected=shared-prefix-0007 actual=<NUL>×17 + "7" ``` Note it is not only the first value of the batch: the corrupted value becomes the `previous` for the next one, so the damage runs to the end of the batch. It is silent — no exception, no warning, just wrong bytes in a CHAR/VARCHAR/BINARY column. ### What changes Keep the value that was just assembled instead of a view into the vector: ```java previous = ByteBuffer.wrap(bytes); ``` `bytes` is already a fresh `new byte[length]` allocated a few lines above, so this copies nothing — it only stops handing out a reference to memory somebody else owns. That `skipBinary` alternates two vectors (`c1`/`c2` swapped at the end of each iteration, with `c1.reset()` at the top) is, I think, the same hazard being avoided by other means in the sibling method — which is why I read the aliasing in `readValues` as an oversight rather than a deliberate trade. ### Why the existing tests are green Every case in `DeltaByteArrayEncodingTest` reads a whole page into one vector in a single call and never resets it: ```java reader.initFromPage(length, writer.getBytes().toInputStream()); reader.readBinary(length, writableColumnVector, 0); ``` With no reset there is nothing to zero the buffer, so the aliased `previous` stays valid and the assertions pass. The skip variants read one value at a time but likewise never reset. The added case is the first that splits a page across batches. ### Blast radius Only the DELTA_BYTE_ARRAY path, and only the line that decides what `previous` points at. Within a single batch the bytes at `b.data[b.offset..]` and `bytes` are identical — `putByteArray` copies `bytes` into the buffer — so a page that fits in one batch decodes exactly as before. `DeltaByteArrayEncodingTest` is 7 tests and the whole `org.apache.paimon.format.parquet` package is 236 tests, 0 failures. ### API and Format No change to any public signature, option or on-disk format. Files already written are unaffected; they simply read back correctly now. -- 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]
