Skip to content

Add fast path for GpuNvl when lhs has no nulls #14050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
binmahone wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add fast path for GpuNvl when lhs has no nulls #14050

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions sql-plugin/src/main/scala/com/nvidia/spark/rapids/nullExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,24 @@ import org.apache.spark.sql.vectorized.ColumnarBatch

object GpuNvl {
def apply(lhs: ColumnVector, rhs: ColumnVector): ColumnVector = {
withResource(lhs.isNotNull) { isLhsNotNull =>
isLhsNotNull.ifElse(lhs, rhs)
// Fast path: if lhs has no nulls, just return lhs directly
if (lhs.getNullCount == 0) {
lhs.incRefCount()
} else {
Comment on lines +32 to +35
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The fast path optimization when lhs has no nulls should have explicit test coverage. Consider adding a test case in conditionals_test.py that exercises the scenario where the first argument to nvl() has nullable=False, to ensure this optimization path is tested. For example, you could add a test like test_nvl with binary_op_df using a data_gen with nullable=False.

Copilot uses AI. Check for mistakes.
withResource(lhs.isNotNull) { isLhsNotNull =>
Copy link
Copy Markdown
Collaborator

@revans2 revans2 Dec 22, 2025

Choose a reason for hiding this comment

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

nit: If we are doing this level of optimization, would it be worth testing to see the performance improvement for using the following APIs instead.

https://github.com/rapidsai/cudf/blob/1d20c668b932cdb864389427fdaef972a5168d03/java/src/main/java/ai/rapids/cudf/ColumnView.java#L536-L548

isLhsNotNull.ifElse(lhs, rhs)
}
}
}

def apply(lhs: ColumnVector, rhs: Scalar): ColumnVector = {
withResource(lhs.isNotNull) { isLhsNotNull =>
isLhsNotNull.ifElse(lhs, rhs)
// Fast path: if lhs has no nulls, just return lhs directly
if (lhs.getNullCount == 0) {
lhs.incRefCount()
} else {
withResource(lhs.isNotNull) { isLhsNotNull =>
isLhsNotNull.ifElse(lhs, rhs)
}
}
}
}
Expand Down