Support IF_NOT_CONTAINED filter type and loading inline deletion vectors for OSS delta [databricks] - #15368
Conversation
…ors for OSS delta Signed-off-by: Jihoon Son <ghoonson@gmail.com>
Greptile SummaryThis PR fixes CDF (Change Data Feed) reads with deletion vectors in the OSS Delta native readers (
Confidence Score: 5/5Safe to merge; the changes are well-scoped, all three reader paths are exercised by integration tests, and no existing callers are broken. The two new behaviours — IF_NOT_CONTAINED row-count math and inline DV byte loading — are each straightforward and verified by dedicated integration tests using assert_gpu_and_cpu_are_equal_collect. The filterTypeOpt field threads cleanly through SpillableDeletionVectorInfo, DeltaParquetExtraInfo, and PerFileDVEntry without touching the GPU decode path. The ByteBufferInputStream extraction adds argument validation without changing semantics for existing callers. No GPU resource leaks, OOM-retry gaps, or cross-shim inconsistencies were found. Files Needing Attention: No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Reader as GPU Delta Reader
participant RDV as RapidsDeletionVectors
participant RDSB as RapidsDVStoredBitmap
participant InMem as RapidsInMemoryDVStore
participant OnDisk as RapidsHadoopDVStore
participant Loader as DeltaSerializedBitmapLoader
Reader->>RDV: loadDeletionVector(fileIO, dvDescOpt, filterTypeOpt, tablePath)
RDV->>RDSB: storedBitmap.load(fileIO)
alt isEmpty
RDSB-->>RDV: serializedEmptyBitmap()
else isInline
RDSB->>InMem: load(dvDescriptor.inlineData)
InMem->>Loader: loadFromBytes(bytes) [no CRC]
Loader-->>InMem: HostMemoryBuffer
InMem-->>RDSB: HostMemoryBuffer
else isOnDisk
RDSB->>OnDisk: load(path, offset, size)
OnDisk->>Loader: load(stream, size) [with CRC]
Loader-->>OnDisk: HostMemoryBuffer
OnDisk-->>RDSB: HostMemoryBuffer
end
RDSB-->>RDV: HostMemoryBuffer (serialized bitmap)
RDV-->>Reader: HostMemoryBuffer
Reader->>RDV: computeNumRowsAlive(totalRows, bitmap, filterTypeOpt, offsets, numRows)
alt IF_CONTAINED
RDV-->>Reader: totalRows - numMarkedRows
else IF_NOT_CONTAINED
RDV-->>Reader: numMarkedRows
else None (no DV)
RDV-->>Reader: totalRows
end
Reader->>Reader: DeletionVectorInfo(bitmap, isIfNotContained, offsets, numRows)
Reader->>Reader: cuDF Parquet read with DV filter
Reviews (9): Last reviewed commit: "address comments" | Re-trigger Greptile |
|
build |
|
NOTE: release/26.08 has been created from main. Please retarget your PR to release/26.08 if it should be included in the release. |
There was a problem hiding this comment.
Pull request overview
Adds missing native-reader support needed for Delta Change Data Feed (CDF) reads on tables with deletion vectors (DVs), specifically handling IF_NOT_CONTAINED row-index filters and inline (in-log) DV payloads, plus targeted test coverage.
Changes:
- Extend DV loading/row-count logic to accept both
IF_CONTAINEDandIF_NOT_CONTAINEDsemantics and propagate that intent into the cuDF DV reader path. - Add inline deletion vector loading support for Delta 33x–41x by parsing inline bitmap bytes into host buffers.
- Add/extend unit + integration tests covering ByteBuffer-backed stream edge cases and Delta CDF scenarios involving mixed row-index filter types.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sql-plugin/src/test/scala/com/nvidia/spark/rapids/ByteBufferInputStreamSuite.scala | Adds unit tests for ByteBuffer-backed InputStream argument/edge-case behavior. |
| sql-plugin/src/main/scala/com/nvidia/spark/rapids/parquet/ParquetCachedBatchSerializer.scala | Switches to shared ByteBufferInputStream implementation (removes local copy). |
| sql-plugin/src/main/scala/com/nvidia/spark/rapids/ByteBufferInputStream.scala | Introduces shared ByteBuffer-backed InputStream with Spark-aligned semantics. |
| integration_tests/src/main/python/delta_lake_test.py | Adds CDF + deletion-vector integration tests, including mixed filter-type scenarios. |
| delta-lake/delta-spark400db173/src/main/scala/com/nvidia/spark/rapids/delta/RapidsDeletionVectors.scala | Adapts DV row counting helper usage to shared “marked rows” semantics. |
| delta-lake/common/src/main/scala/com/nvidia/spark/rapids/delta/RapidsDeletionVectorRowCountUtils.scala | Renames/counts bitmap “marked rows” (vs “deleted rows”) to support both filter semantics. |
| delta-lake/common/src/main/delta-33x-41x/scala/org/apache/spark/sql/delta/deletionvectors/RapidsStoredBitmap.scala | Adds inline DV loading path and wires DV store creation through RapidsFileIO. |
| delta-lake/common/src/main/delta-33x-41x/scala/org/apache/spark/sql/delta/deletionvectors/RapidsDeletionVectorStore.scala | Adds inline bitmap loader + refactors loaders to support non-CRC inline parsing. |
| delta-lake/common/src/main/delta-33x-41x/scala/com/nvidia/spark/rapids/delta/common/RapidsDeletionVectors.scala | Accepts IF_NOT_CONTAINED, adds alive-row counting based on filter semantics, and exposes helper flagging the filter type. |
| delta-lake/common/src/main/delta-33x-41x/scala/com/nvidia/spark/rapids/delta/common/GpuDeltaParquetFileFormatBase2.scala | Propagates filter-type into DV metadata and uses new alive-row computation for partition routing and reader metadata. |
|
build |
|
This PR is currently blocked by #15408. |
|
build |
| @@ -110,17 +114,17 @@ object RapidsDeletionVectors extends Logging { | |||
| if (dvDescriptorOpt.isDefined && filterTypeOpt.isDefined) { | |||
There was a problem hiding this comment.
Could we pattern-match on (dvDescriptorOpt, filterTypeOpt) here instead of checking isDefined and then calling .get? An exhaustive tuple match would encode the both-defined-or-both-absent invariant directly and avoid unsafe access. Since loadScalaBitmap repeats the same validation, a small shared helper could also remove that duplication.
There was a problem hiding this comment.
Fixed as suggested.
| * Loads a bitmap payload and validates its trailing checksum. The CRC is initialized | ||
| * with the magic number before this method is called. | ||
| */ | ||
| def loadAsStandardFormat(input: DataInputStream, size: Int, crc: CRC32): HostMemoryBuffer |
There was a problem hiding this comment.
Since this is a private trait and both implementations immediately forward these overloads to an Option[CRC32] implementation, could the trait expose a single crcOpt: Option[CRC32] method? Callers can pass Some(crc) or None, eliminating the four forwarding methods.
There was a problem hiding this comment.
Fixed as suggested.
| extends InputStream { | ||
|
|
||
| override def read(): Int = { | ||
| if (buffer == null || buffer.remaining() == 0) { |
There was a problem hiding this comment.
Nit: !buffer.hasRemaining is the idiomatic NIO spelling for buffer.remaining() == 0; likewise at lines 50 and 64.
|
|
||
| def loadFromBytes(bytes: Array[Byte]): HostMemoryBuffer = { | ||
| val bb = ByteBuffer.wrap(bytes) | ||
| bb.order(ByteOrder.LITTLE_ENDIAN) |
There was a problem hiding this comment.
Nit: this can be constructed in one expression: val bb = ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN).
| bb.order(ByteOrder.LITTLE_ENDIAN) | ||
| val magicNumber = bb.getInt() | ||
| val remainingSize = bb.remaining() | ||
| withResource(new ByteBufferInputStream(bb)) { bais => |
There was a problem hiding this comment.
Nit: the outer resource wrapper can be removed by constructing new DataInputStream(new ByteBufferInputStream(bb)) in a single withResource; closing the DataInputStream closes its underlying stream.
|
Thanks @gerashegalov. I addressed all your comments. |
|
Just a question else LGTM. |
|
build |
Resolves the latest merge conflicts in #15412 after #15423 merged. ### Description Merge the current `release/26.08` head into the current `main` head. The initial conflicts were the root and Scala 2.13 project versions. Both were resolved by retaining main's `26.10.0-SNAPSHOT` version instead of the release branch's `26.08.0-SNAPSHOT` version. The branch was refreshed again after additional PRs landed on `release/26.08`. The current diff includes all release updates added after #15423, including: - #15413 — preserve Spark 4.2 BroadcastHashJoin `isSkewJoin` - #15422 — fix Iceberg REST S3 path regression coverage - #15368 — OSS Delta deletion-vector updates - #15411 — fix OSS Delta RTAS on Spark 4.x+ - #15416 — match Spark 4.2 `date_trunc` overflow behavior ### Checklists Documentation - [ ] Updated for new or modified user-facing features or behaviors - [x] No user-facing change Testing - [ ] Added or modified tests to cover new code paths - [x] Covered by existing tests (The included release commits retain their original tests.) - [ ] Not required Performance - [ ] Tests ran and results are added in the PR description - [ ] Issue filed with a link in the PR description - [x] Not required ### Validation - `git diff --check` - Parsed both initially resolved POM files as XML - `python3 -m py_compile` for the modified Iceberg, Delta, and date-time integration tests IMPORTANT: Merge this PR using **Create a merge commit** so the release commit ancestry is preserved and #15412 can close automatically. --------- Signed-off-by: Sameer Raheja <sraheja@.nvidia.com> Signed-off-by: Rahul Prabhu <raprabhu@nvidia.com> Signed-off-by: Chong Gao <chongg@nvidia.com> Signed-off-by: Firestarman <firestarmanllc@gmail.com> Signed-off-by: Ray Liu <liurenjie2008@gmail.com> Signed-off-by: liyuan <yuali@nvidia.com> Signed-off-by: Jihoon Son <ghoonson@gmail.com> Signed-off-by: Niranjan Artal <nartal@nvidia.com> Co-authored-by: Sameer Raheja <sameerz@users.noreply.github.com> Co-authored-by: Sameer Raheja <sraheja@.nvidia.com> Co-authored-by: Gary Shen <gashen@nvidia.com> Co-authored-by: Rahul Prabhu <100436830+sdrp713@users.noreply.github.com> Co-authored-by: Chong Gao <chongg@nvidia.com> Co-authored-by: Chong Gao <res_life@163.com> Co-authored-by: Liangcai Li <firestarmanllc@gmail.com> Co-authored-by: Renjie Liu <liurenjie2008@gmail.com> Co-authored-by: Jihoon Son <ghoonson@gmail.com> Co-authored-by: Niranjan Artal <50492963+nartal1@users.noreply.github.com>
Fixes #15326.
Description
The CDF read with deletion vectors currently fails. Two things were missing to support this case:
IF_NOT_CONTAINEDrow index filter type support. The Delta CDC reader can use this type of row index filter.This PR adds those supports based on NVIDIA/cudf#23402 for OSS Delta. The plugin now can load inline deletion vectors and process the
IF_NOT_CONTAINEDfilter properly with all 3 Delta readers.Note that the issue exists only with the native readers (
GpuDeltaParquetFileFormatBase2). The legacy reader (GpuDeltaParquetFileFormatBase) does not have this issue.Databricks readers have the same issue, and will be fixed in #15365.
Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance