Use configured copy buffer for Hadoop vectored reads [databricks] - #15242
Conversation
Forward-port NVIDIA#15164 to main. Signed-off-by: Hongbin Ma <mahongbin@apache.org>
Greptile SummaryThis PR adds a Hadoop-specific copy-buffer path for fallback vectored reads. The main changes are:
Confidence Score: 5/5This looks safe to merge after a small hardening cleanup.
sql-plugin/src/main/java/com/nvidia/spark/rapids/fileio/hadoop/HadoopInputFile.java Important Files Changed
Reviews (1): Last reviewed commit: "Use configured copy buffer for Hadoop ve..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (copyRanges.isEmpty()) { | ||
| return; | ||
| } | ||
| byte[] copyBuffer = new byte[copyBufferSize]; |
There was a problem hiding this comment.
copyBufferSize comes directly from the Hadoop parquet.read.allocation.size setting and is allocated on every vectored read. If that setting is accidentally set very large, each fallback Parquet read can allocate a huge on-heap byte array and fail the executor with an allocation error before any data is copied.
There was a problem hiding this comment.
Pull request overview
This PR updates the Hadoop-backed RapidsInputFile fallback for vectored reads so it uses a configurable copy buffer size (via parquet.read.allocation.size) rather than relying on a small fixed internal copy chunk, addressing the remote Parquet scan performance regression described in #15163.
Changes:
- Add a
HadoopInputFile.readVectored(...)override that usesRapidsInputFile.readVectoredUsingCopyBuffer(...)with a per-call temporary byte buffer. - Read the copy buffer size from Hadoop
Configuration(parquet.read.allocation.size), defaulting toRapidsInputFile.DEFAULT_READ_VECTORED_COPY_BUFFER_SIZE, and validate it is positive.
## Description This is the `main` forward-port of #4765, which targeted `release/26.06`. It updates the default `RapidsInputFile.readVectored` fallback to copy through an explicit byte-buffer loop and write into the destination `HostMemoryBuffer` with `setBytes`: - allocate a bounded per-call buffer using the smaller of 8 MiB and the largest requested range - expose `RapidsInputFile.readVectoredUsingCopyBuffer(..., byte[])` for callers with a well-defined scratch-buffer lifecycle - keep validation and stream-copy details in a package-private utility - avoid retained shared buffers and `ThreadLocal` state Related issue: NVIDIA/cudf-spark#15163. The corresponding cudf-spark `main` forward-port, NVIDIA/cudf-spark#15242, uses the caller-supplied-buffer helper in `HadoopInputFile.readVectored`, allowing Hadoop-backed fallback reads to honor `parquet.read.allocation.size` while preserving the `inputFile.readVectored` abstraction. ## Testing The current `main` forward-port commit (`daca8177f626c4c48676beea5f16d36a974620fe`) was tested on an x86 host with: ```text mvn -B -Dmaven.antrun.skip=true -Dtest=RapidsInputFileTest \ test-compile surefire:test@default-test ``` Result: ```text Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` The JNI `26.08.0-SNAPSHOT` artifact was then installed locally and used to package the corresponding cudf-spark `main` forward-port: ```text mvn -B -pl sql-plugin -am -DskipTests \ -Dspark-rapids-jni.version=26.08.0-SNAPSHOT package ``` Result: `BUILD SUCCESS` for all four reactor modules. ## Performance Validation End-to-end NDS validation of the same implementation is documented in NVIDIA/cudf-spark#15163. Five interleaved runs per side did not reproduce a stable regression with either PerfIO disabled or enabled. Signed-off-by: Hongbin Ma <mahongbin@apache.org>
binmahone
changed the title
|
build |
| return new HadoopInputStream(fs.open(filePath)); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
nit [non-blocking]: The override silently drops the null-guards that the interface default provides. A null copyRanges will NPE on .isEmpty() without a helpful message, and a null output will fail deep inside RapidsInputFileUtils. Consider adding the same two guards from the default:
Objects.requireNonNull(output, "output can't be null");
Objects.requireNonNull(copyRanges, "copyRanges can't be null");
|
build |
Fixes #15163.
Description
This is the
mainforward-port of #15164, which targetedrelease/26.06.When an optimized input-file implementation is not selected, remote Parquet reads use
HadoopInputFileand the genericRapidsInputFile.readVectoredfallback. Before NVIDIA/cudf-spark-jni#4765, that fallback usedHostMemoryBuffer.copyFromStreamwith a 128 KiB internal copy chunk, while the pre-#14674 Hadoop copy loop usedparquet.read.allocation.sizewith an 8 MiB default.This PR overrides
HadoopInputFile.readVectoredto use the caller-supplied-buffer helper from the JNImainforward-port, NVIDIA/cudf-spark-jni#4808:parquet.read.allocation.sizereadVectoredcallThreadLocalstateGpuParquetScanon theinputFile.readVectoredabstraction introduced by optimize iceberg read #14674The measured regression was on the S3A fallback with PerfIO disabled. The same
HadoopInputFilefallback is also used by GCS and other Hadoop-backed filesystems that do not provide their own optimizedreadVectoredimplementation. When S3 PerfIO is enabled,S3InputFile.readVectoredcontinues to use the optimized PerfIO path and bypasses this fallback.Requires NVIDIA/cudf-spark-jni#4808. The previous
release/26.06implementation is #15164 and NVIDIA/cudf-spark-jni#4765.Testing
The JNI
mainforward-port (daca8177f626c4c48676beea5f16d36a974620fe) passed all eightRapidsInputFileTesttests. After installing that JNI26.08.0-SNAPSHOTartifact locally, this commit (5ec2c7ffc1763d5b141e20066ab303e9c287e962) was packaged on an x86 host with:Result:
BUILD SUCCESSfor all four reactor modules.Five interleaved full-NDS runs per configuration did not reproduce a stable performance regression after the fix, with either PerfIO disabled or enabled. See #15163 for the complete environment, per-run results, and analysis.
Checklists
Documentation
Testing
Performance