You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "iemejia (via GitHub)" <gi...@apache.org> on 2026/04/19 11:31:29 UTC
[PR] GH-3493: Optimize PlainValuesReader with direct ByteBuffer reads [parquet-java]
iemejia opened a new pull request, #3494:
URL: https://github.com/apache/parquet-java/pull/3494
### Rationale for this change
Closes #3493.
`PlainValuesReader` (used for PLAIN-encoded INT32, INT64, FLOAT, and DOUBLE columns, and for decoding the dictionary page of every dictionary-encoded numeric column) currently reads each value through a `LittleEndianDataInputStream` wrapper around a `ByteBufferInputStream`. Per value, `readInt()` performs 4 separate virtual `in.read()` calls and reassembles the result with bit shifts. The `LittleEndianDataInputStream.readInt()` method itself carries a TODO comment from years ago suggesting exactly this kind of replacement.
### What changes are included in this PR?
**Commit 1** — `Optimize PlainValuesReader with direct ByteBuffer reads`
In `PlainValuesReader.initFromPage()`, obtain the page data as a single contiguous `ByteBuffer` via `stream.slice(stream.available())` with `ByteOrder.LITTLE_ENDIAN`, and call the corresponding `ByteBuffer` accessor directly per value:
- `readInteger()` → `buffer.getInt()`
- `readLong()` → `buffer.getLong()`
- `readFloat()` → `buffer.getFloat()`
- `readDouble()` → `buffer.getDouble()`
- `skip(n)` → `buffer.position(buffer.position() + n * typeSize)`
`ByteBuffer.getInt()` with the appropriate byte order is a HotSpot intrinsic that compiles to a single unaligned load instruction on x86/ARM — no virtual dispatch, no per-byte assembly, no checked `IOException` on the per-value path. `ByteBufferInputStream.slice()` already handles both single-buffer (zero-copy view) and multi-buffer (single contiguous copy) cases transparently.
**Commit 2** — `Deprecate LittleEndianDataInputStream and migrate last test usage`
After commit 1, `LittleEndianDataInputStream` has no remaining production usages. This commit:
- Adds `@Deprecated` and detailed javadoc pointing to the faster `ByteBuffer` + `LITTLE_ENDIAN` alternative
- Migrates the only remaining usage in `TestColumnChunkPageWriteStore.intValue()` to `BytesInput.toByteBuffer().order(LITTLE_ENDIAN).getInt()`, eliminating a `BytesInput → ByteArrayOutputStream → ByteArrayInputStream → LittleEndianDataInputStream` round-trip
The class is left in place (only `@Deprecated`) for source/binary compatibility of any downstream code that may still reference it. It can be removed in a future major release.
### Benchmark results
`IntEncodingBenchmark.decodePlain` (100,000 INT32 values per invocation, JMH `-wi 3 -i 5 -f 1`):
| Pattern | Before (ops/s) | After (ops/s) | Speedup |
|------------------|---------------:|----------------:|--------:|
| SEQUENTIAL | 92,918,297 | 1,143,149,235 | **12.3x** |
| RANDOM | 92,126,888 | 1,147,547,093 | **12.5x** |
| LOW_CARDINALITY | 93,005,451 | 1,142,666,760 | **12.3x** |
| HIGH_CARDINALITY | 93,312,596 | 1,144,681,876 | **12.3x** |
The speedup is consistent across data patterns because the bottleneck is entirely in the per-value dispatch overhead, not the data itself. All four numeric plain reader types (int, long, float, double) benefit equally.
### Are these changes tested?
Yes. All 573 `parquet-column` and 308 `parquet-common` tests pass. The migrated `TestColumnChunkPageWriteStore.testColumnOrderV1` test passes; the two pre-existing `getSubject` failures in `TestColumnChunkPageWriteStore` on JDK 18+ are unrelated and reproduce on `master` without these changes.
### Are there any user-facing changes?
`LittleEndianDataInputStream` is now `@Deprecated`. No behavioral or binary-compatibility changes for existing callers.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] GH-3493: Optimize PlainValuesReader with direct ByteBuffer reads [parquet-java]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3494:
URL: https://github.com/apache/parquet-java/pull/3494#discussion_r3156914189
##########
parquet-column/src/main/java/org/apache/parquet/column/values/plain/PlainValuesReader.java:
##########
@@ -19,120 +19,92 @@
package org.apache.parquet.column.values.plain;
import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
import org.apache.parquet.bytes.ByteBufferInputStream;
import org.apache.parquet.bytes.LittleEndianDataInputStream;
import org.apache.parquet.column.values.ValuesReader;
-import org.apache.parquet.io.ParquetDecodingException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Plain encoding for float, double, int, long
+ * Plain encoding for float, double, int, long.
+ *
+ * <p>Reads directly from a {@link ByteBuffer} with {@link ByteOrder#LITTLE_ENDIAN} byte order,
+ * bypassing the {@link LittleEndianDataInputStream} wrapper to avoid per-value virtual dispatch
+ * overhead. The underlying page data is obtained as a single contiguous {@link ByteBuffer} via
+ * {@link ByteBufferInputStream#slice(int)}.
*/
public abstract class PlainValuesReader extends ValuesReader {
private static final Logger LOG = LoggerFactory.getLogger(PlainValuesReader.class);
- protected LittleEndianDataInputStream in;
+ ByteBuffer buffer;
@Override
public void initFromPage(int valueCount, ByteBufferInputStream stream) throws IOException {
LOG.debug("init from page at offset {} for length {}", stream.position(), stream.available());
- this.in = new LittleEndianDataInputStream(stream.remainingStream());
+ int available = stream.available();
+ if (available > 0) {
+ this.buffer = stream.slice(available).order(ByteOrder.LITTLE_ENDIAN);
+ } else {
+ this.buffer = ByteBuffer.allocate(0).order(ByteOrder.LITTLE_ENDIAN);
Review Comment:
Should we introduce a constant:
```
private static final ByteBuffer EMPTY = ByteBuffer.allocate(0).order(ByteOrder.LITTLE_ENDIAN);
```
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] GH-3493: Optimize PlainValuesReader with direct ByteBuffer reads [parquet-java]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia closed pull request #3494: GH-3493: Optimize PlainValuesReader with direct ByteBuffer reads
URL: https://github.com/apache/parquet-java/pull/3494
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] GH-3493: Optimize PlainValuesReader with direct ByteBuffer reads [parquet-java]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on PR #3494:
URL: https://github.com/apache/parquet-java/pull/3494#issuecomment-4472734637
Closing in favor of #3565.
I initially submitted a series of small, focused PRs thinking they'd be easier to review. In practice the sheer number (~16 PRs, with more pending) made things harder to follow — even for me. I've regrouped the changes by encoding type / performance area so that each PR is self-contained with its own benchmarks and test coverage, which should make review and performance analysis much more straightforward.
Apologies for the churn. If you've been reviewing this PR, please continue the discussion on #3565 which supersedes it. Thank you.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] GH-3493: Optimize PlainValuesReader with direct ByteBuffer reads [parquet-java]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3494:
URL: https://github.com/apache/parquet-java/pull/3494#discussion_r3156907453
##########
parquet-column/src/main/java/org/apache/parquet/column/values/plain/PlainValuesReader.java:
##########
@@ -19,120 +19,92 @@
package org.apache.parquet.column.values.plain;
import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
import org.apache.parquet.bytes.ByteBufferInputStream;
import org.apache.parquet.bytes.LittleEndianDataInputStream;
import org.apache.parquet.column.values.ValuesReader;
-import org.apache.parquet.io.ParquetDecodingException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Plain encoding for float, double, int, long
+ * Plain encoding for float, double, int, long.
+ *
+ * <p>Reads directly from a {@link ByteBuffer} with {@link ByteOrder#LITTLE_ENDIAN} byte order,
+ * bypassing the {@link LittleEndianDataInputStream} wrapper to avoid per-value virtual dispatch
+ * overhead. The underlying page data is obtained as a single contiguous {@link ByteBuffer} via
+ * {@link ByteBufferInputStream#slice(int)}.
*/
public abstract class PlainValuesReader extends ValuesReader {
private static final Logger LOG = LoggerFactory.getLogger(PlainValuesReader.class);
- protected LittleEndianDataInputStream in;
+ ByteBuffer buffer;
@Override
public void initFromPage(int valueCount, ByteBufferInputStream stream) throws IOException {
LOG.debug("init from page at offset {} for length {}", stream.position(), stream.available());
- this.in = new LittleEndianDataInputStream(stream.remainingStream());
+ int available = stream.available();
+ if (available > 0) {
+ this.buffer = stream.slice(available).order(ByteOrder.LITTLE_ENDIAN);
+ } else {
+ this.buffer = ByteBuffer.allocate(0).order(ByteOrder.LITTLE_ENDIAN);
+ }
}
@Override
public void skip() {
skip(1);
}
- void skipBytesFully(int n) throws IOException {
- int skipped = 0;
- while (skipped < n) {
- skipped += in.skipBytes(n - skipped);
- }
- }
-
public static class DoublePlainValuesReader extends PlainValuesReader {
@Override
public void skip(int n) {
- try {
- skipBytesFully(n * 8);
- } catch (IOException e) {
- throw new ParquetDecodingException("could not skip " + n + " double values", e);
- }
+ buffer.position(buffer.position() + n * 8);
Review Comment:
When skipping, should we validate bounds?
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] GH-3493: Optimize PlainValuesReader with direct ByteBuffer reads [parquet-java]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3494:
URL: https://github.com/apache/parquet-java/pull/3494#discussion_r3156902887
##########
parquet-column/src/main/java/org/apache/parquet/column/values/plain/PlainValuesReader.java:
##########
@@ -19,120 +19,92 @@
package org.apache.parquet.column.values.plain;
import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
import org.apache.parquet.bytes.ByteBufferInputStream;
import org.apache.parquet.bytes.LittleEndianDataInputStream;
import org.apache.parquet.column.values.ValuesReader;
-import org.apache.parquet.io.ParquetDecodingException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Plain encoding for float, double, int, long
+ * Plain encoding for float, double, int, long.
+ *
+ * <p>Reads directly from a {@link ByteBuffer} with {@link ByteOrder#LITTLE_ENDIAN} byte order,
+ * bypassing the {@link LittleEndianDataInputStream} wrapper to avoid per-value virtual dispatch
+ * overhead. The underlying page data is obtained as a single contiguous {@link ByteBuffer} via
+ * {@link ByteBufferInputStream#slice(int)}.
*/
public abstract class PlainValuesReader extends ValuesReader {
private static final Logger LOG = LoggerFactory.getLogger(PlainValuesReader.class);
- protected LittleEndianDataInputStream in;
+ ByteBuffer buffer;
Review Comment:
```suggestion
private ByteBuffer buffer;
```
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org