-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-57902][SQL] Emit a ternary for 2-arg Coalesce with a non-nullable pure fallback #56971
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.
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
Changes from all commits
8a45067
3b24687
2f0da82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import java.sql.Timestamp | |
|
|
||
| import org.apache.spark.{SparkFunSuite, SparkRuntimeException} | ||
| import org.apache.spark.sql.catalyst.FunctionIdentifier | ||
| import org.apache.spark.sql.catalyst.InternalRow | ||
| import org.apache.spark.sql.catalyst.analysis.{FunctionRegistry, SimpleAnalyzer, UnresolvedAttribute} | ||
| import org.apache.spark.sql.catalyst.expressions.codegen.CodegenContext | ||
| import org.apache.spark.sql.catalyst.expressions.objects.AssertNotNull | ||
|
|
@@ -207,10 +208,41 @@ class NullExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper { | |
|
|
||
| test("SPARK-22705: Coalesce should use less global variables") { | ||
| val ctx = new CodegenContext() | ||
| Coalesce(Seq(Literal("a"), Literal("b"))).genCode(ctx) | ||
| // Use three children so that the general do-while codegen path is exercised: the two-argument | ||
| // form with a non-nullable pure fallback is now special-cased to a ternary with no global | ||
| // state (covered by the test below). | ||
| Coalesce(Seq(Literal("a"), Literal("b"), Literal("c"))).genCode(ctx) | ||
| assert(ctx.inlinedMutableStates.size == 1) | ||
| } | ||
|
|
||
| test("Coalesce with a non-nullable pure fallback compiles to a ternary with no global state") { | ||
| val ctx = new CodegenContext() | ||
| Coalesce(Seq(Literal.create(null, StringType), Literal("b"))).genCode(ctx) | ||
| assert(ctx.inlinedMutableStates.isEmpty) | ||
| checkEvaluation(Coalesce(Seq(Literal.create(null, StringType), Literal("a"))), "a") | ||
| checkEvaluation(Coalesce(Seq(Literal("x"), Literal("y"))), "x") | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the subtlest branch, which is the probe-reuse path (2-arg, non-nullable b, but b.code NON-empty, so Some(Seq(first, second)) is threaded into the general path), is only covered indirectly by end-to-end suites, not by a direct unit test. Adding a direct regression test would be nice here too. |
||
|
|
||
| test("Coalesce ternary fast path over a BoundReference first arg and a primitive fallback") { | ||
| // The motivating hot path is coalesce(nullable_column, literal): the first arg is a | ||
| // BoundReference read from the input row (a distinct codegen path from a Literal), and the | ||
| // fallback is a non-nullable primitive that emits no code, so the ternary fast path applies. | ||
| val coalesce = Coalesce(Seq(BoundReference(0, IntegerType, nullable = true), Literal(0))) | ||
| checkEvaluation(coalesce, 42, InternalRow(42)) | ||
| checkEvaluation(coalesce, 0, InternalRow(null)) | ||
| } | ||
|
|
||
| test("Coalesce reuses the probed ExprCode when the non-nullable fallback emits code") { | ||
| // A two-arg coalesce whose non-nullable fallback is itself a BoundReference: its code is | ||
| // non-empty, so the ternary fast path does not apply and the probed ExprCodes are threaded | ||
| // into the general do-while path (rather than generating the children a second time). | ||
| val coalesce = Coalesce(Seq( | ||
| BoundReference(0, IntegerType, nullable = true), | ||
| BoundReference(1, IntegerType, nullable = false))) | ||
| checkEvaluation(coalesce, 42, InternalRow(42, 7)) | ||
| checkEvaluation(coalesce, 7, InternalRow(null, 7)) | ||
| } | ||
|
|
||
| test("AtLeastNNonNulls should not throw 64KiB exception") { | ||
| val inputs = (1 to 4000).map(x => Literal(s"x_$x")) | ||
| checkEvaluation(AtLeastNNonNulls(1, inputs), true) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting a minor test coverage gap: the new fast-path test uses only Literal inputs and StringType. The motivating hot path is coalesce(nullable_column, 0) where the first arg is a BoundReference (a distinct codegen path via currentVars lookup, not ExprCode.forNonNullValue) and the fallback is a primitive.
Adding a BoundReference-through-a-row case (both null and non-null branches, e.g. checkEvaluation(Coalesce(Seq(BoundReference(0, IntegerType, nullable = true), Literal(0))), 42, InternalRow(42))) would pin the actual motivating path.