Skip to content

Fix a special case in limit where it could return an empty batch with the wrong number of columns (#13938) - #13971

Merged
res-life merged 1 commit into
NVIDIA:release/25.12from
res-life:pick
Dec 8, 2025
Merged

Fix a special case in limit where it could return an empty batch with the wrong number of columns (#13938)#13971
res-life merged 1 commit into
NVIDIA:release/25.12from
res-life:pick

Conversation

@res-life

@res-life res-life commented Dec 8, 2025

Copy link
Copy Markdown
Collaborator

Picked from main branch(#13938)

PR #13903 (target branch 25.12) needs to use the CSV files in #13938

… the wrong number of columns (NVIDIA#13938)

This Fixes NVIDIA#13914

### Description

The code was wrong, but not so wrong that unless we broadcast the empty
batch it would probably just be filtered out and ignored.

### Checklists

- [ ] This PR has added documentation for new or modified features or
behaviors.
- [X] This PR has added new tests or modified existing tests to cover
new code paths.
(Please explain in the PR description how the new code paths are tested,
such as names of the new/existing tests that cover them.)
- [ ] Performance testing has been performed and its results are added
in the PR description. Or, an issue has been filed with a link in the PR
description.

---------

Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
@res-life

res-life commented Dec 8, 2025

Copy link
Copy Markdown
Collaborator Author

build

@greptile-apps

greptile-apps Bot commented Dec 8, 2025

Copy link
Copy Markdown
Contributor

Greptile Overview

Greptile Summary

Fixes a bug where GpuBaseLimitIterator could return an empty batch with the wrong number of columns when the offset exceeded all available rows in a partition.

  • Root cause: The original code captured numCols from the first input batch, but when all batches were consumed during offset processing and an empty batch needed to be returned, it created an incorrect empty batch using ArrayBuffer[GpuColumnVector](numCols) which had no actual column data types
  • The fix: Passes the expected output dataTypes array to GpuBaseLimitIterator and uses GpuColumnVector.emptyBatch(StructType(fields)) to create properly-typed empty batches
  • Impact: This bug manifested when the empty batch was broadcast (e.g., in cross joins with an empty side), causing column count mismatches
  • Testing: Added integration test test_empty_cross_side_with_limit that reproduces the scenario with a cross join between an empty table and a non-empty table with different schemas

Confidence Score: 5/5

  • This PR is safe to merge - it's a targeted bug fix with proper test coverage
  • The fix is well-scoped, addressing a specific edge case in limit processing. The change follows established patterns in the codebase (passing dataTypes array), includes both unit and integration tests, and the code logic is straightforward and correct.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
sql-plugin/src/main/scala/com/nvidia/spark/rapids/limit.scala 5/5 Core fix: Added dataTypes parameter to GpuBaseLimitIterator to properly construct empty batches with correct schema instead of deriving from input batch
tests/src/test/scala/com/nvidia/spark/rapids/LimitRetrySuite.scala 5/5 Updated test to pass new dataTypes parameter to GpuBaseLimitIterator constructor, also updated copyright year
integration_tests/src/main/python/join_test.py 5/5 Added new integration test test_empty_cross_side_with_limit that reproduces the bug with cross join of empty table
integration_tests/src/test/resources/t0.csv 5/5 Test data file with header-only CSV (two columns: c0, c1) representing empty table for reproducing the bug
integration_tests/src/test/resources/t1.csv 5/5 Test data file with one column (c0) and one row (true) for cross join test scenario

Sequence Diagram

sequenceDiagram
    participant Client as Spark Client
    participant Limit as GpuBaseLimitIterator
    participant Batch as Input Batch Iterator
    participant Empty as GpuColumnVector.emptyBatch

    Client->>Limit: next()
    Limit->>Batch: input.next()
    Batch-->>Limit: batch (may be null or empty)
    
    alt offset >= all rows in partition
        Note over Limit: All batches consumed during offset skip
        Limit->>Empty: emptyBatch(StructType(fields))
        Note over Empty: Creates empty batch with<br/>correct schema from dataTypes
        Empty-->>Limit: ColumnarBatch with correct columns
    else normal case
        Limit->>Limit: slice batch
        Limit-->>Client: ColumnarBatch with data
    end
    
    Limit-->>Client: ColumnarBatch
Loading

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@res-life
res-life requested a review from firestarman December 8, 2025 09:15
@firestarman
firestarman requested a review from revans2 December 8, 2025 10:18
@firestarman

Copy link
Copy Markdown
Collaborator

LGTM since this is just a cherry-pick.

@res-life
res-life merged commit ca5688b into NVIDIA:release/25.12 Dec 8, 2025
65 of 66 checks passed
@res-life
res-life deleted the pick branch December 8, 2025 14:25
@sameerz sameerz added the bug Something isn't working label Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants