[SPARK-48719][SQL] Fix the calculation bug of `RegrSlope` & `RegrIntercept` when the first parameter is null #47105

wayneguow · 2024-06-26T16:43:28Z

What changes were proposed in this pull request?

This PR aims to fix the calculation bug of RegrSlope&RegrIntercept when the first parameter is null. Regardless of whether the first parameter(y) or the second parameter(x) is null, this tuple should be filtered out.

Why are the changes needed?

Fix bug.

Does this PR introduce any user-facing change?

Yes, the calculation changes when the first value of a tuple is null, but the value is truly correct.

How was this patch tested?

Pass GA and test with build/sbt "~sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z linear-regression.sql"

Was this patch authored or co-authored using generative AI tooling?

No.

wayneguow · 2024-06-26T16:49:04Z

sql/core/src/test/resources/sql-tests/results/linear-regression.sql.out

@@ -1,7 +1,7 @@
 -- Automatically generated by SQLQueryTestSuite
 -- !query
 CREATE OR REPLACE TEMPORARY VIEW testRegression AS SELECT * FROM VALUES
-(1, 10, null), (2, 10, 11), (2, 20, 22), (2, 25, null), (2, 30, 35)
+(1, 10, null), (2, 10, 11), (2, 20, 22), (2, 25, null), (2, 30, 35), (2, null, 40)


A tuple is added with the value of y is null, it should be filtered out during calculation, so the output related to RegrSlope & RegrIntercept in the output remains unchanged.

wayneguow · 2024-06-27T06:19:33Z

cc @beliefer

wayneguow · 2024-06-28T07:37:53Z

Gentle ping @HyukjinKwon, when you have time.

LuciferYang · 2024-07-03T07:08:49Z

seems ok, cc @beliefer and @cloud-fan

cloud-fan · 2024-07-03T07:33:32Z

...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/linearRegression.scala

@@ -311,7 +312,8 @@ case class RegrIntercept(left: Expression, right: Expression) extends Declarativ

  private val covarPop = new CovPopulation(right, left)

-  private val varPop = new VariancePop(right)
+  private val varPop = new VariancePop(If(And(IsNotNull(left), IsNotNull(right)),


instead of adding null check to the underlying VariancePop, shall we add it to RegrIntercept? e.g. we can add null check to updateExpressions.

I tried changing it locally and related test cases passed. It seems that the logic is more complicated. Is it worth updating like this? WDYT @cloud-fan

override lazy val updateExpressions: Seq[Expression] = { val isNull = left.isNull || right.isNull val updateResult = covarPop.updateExpressions ++ varPop.updateExpressions aggBufferAttributes.zip(updateResult).map { case (oldValue, newValue) => If(isNull, oldValue, newValue) } }

I think it's more efficient, as we do null check earlier, before we execute VariancePop

Actually, the value is calculated regardless of whether it is null with code val updateResult = covarPop.updateExpressions ++ varPop.updateExpressions.

It's similar to the updateExpressionsDef func in Covariance. The value is calculated first, and finally different values are returned depending on whether it is null.

I haven't found a better way to implement this logic. If can use if else directly, it may not be calculated in advance, but it cannot be expressed directly here.

I updated it, would you like to take another look? Thank you. @cloud-fan

cloud-fan · 2024-07-05T07:47:32Z

...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/linearRegression.scala

+    val isNull = left.isNull || right.isNull
+    val updateResult = covarPop.updateExpressions ++ varPop.updateExpressions
+    aggBufferAttributes.zip(updateResult).map { case (oldValue, newValue) =>
+      If(isNull, oldValue, newValue)


we only need to add this for varPop.updateExpressions

the code should be something like

covarPop.updateExpressions ++ varPop.updateExpressions.zip(varPop.aggBufferAttributes)...

BTW to add a bit more color to my proposal: the update expressions of VariancePop is

trimHigherOrder(Seq( If(child.isNull, n, newN), If(child.isNull, avg, newAvg), If(child.isNull, m2, newM2), If(child.isNull, m3, newM3), If(child.isNull, m4, newM4) ))

The child appears not only in the if conditions, but also newAvg, newM2, etc. It's better to not add the extra null check to the child, as it will be duplicated.

Good insight.

cloud-fan · 2024-07-05T08:19:57Z

...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/linearRegression.scala

+  override lazy val updateExpressions: Seq[Expression] = {
+    val isNull = left.isNull || right.isNull
+    covarPop.updateExpressions ++ varPop.updateExpressions.zip(varPop.aggBufferAttributes).map {
+      case (newValue, oldValue) => If(isNull, oldValue, newValue)


let's add some code comments to explain the null check

cloud-fan · 2024-07-05T08:20:03Z

...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/linearRegression.scala

+  override lazy val updateExpressions: Seq[Expression] = {
+    val isNull = left.isNull || right.isNull
+    covarPop.updateExpressions ++ varPop.updateExpressions.zip(varPop.aggBufferAttributes).map {
+      case (newValue, oldValue) => If(isNull, oldValue, newValue)


cloud-fan · 2024-07-05T14:21:38Z

thanks, merging to master!

cloud-fan · 2024-07-05T14:23:04Z

can you open a backport PR for 3.5?

wayneguow · 2024-07-05T14:26:45Z

can you open a backport PR for 3.5?

Ok, let me do it. Thank you for review. @cloud-fan

…rcept` when the first parameter is null ### What changes were proposed in this pull request? This PR aims to fix the calculation bug of `RegrSlope`&`RegrIntercept` when the first parameter is null. Regardless of whether the first parameter(y) or the second parameter(x) is null, this tuple should be filtered out. ### Why are the changes needed? Fix bug. ### Does this PR introduce _any_ user-facing change? Yes, the calculation changes when the first value of a tuple is null, but the value is truly correct. ### How was this patch tested? Pass GA and test with `build/sbt "~sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z linear-regression.sql"` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47105 from wayneguow/SPARK-48719. Authored-by: Wei Guo <guow93@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

handle y null

e5dbb7c

github-actions bot added the SQL label Jun 26, 2024

wayneguow commented Jun 26, 2024

View reviewed changes

wayneguow changed the title ~~[SPARK-48719][SQL] Fix the calculation bug of RegrSlope&RegrIntercept` when the first parameter is null~~ [SPARK-48719][SQL] Fix the calculation bug of RegrSlope & RegrIntercept when the first parameter is null Jun 26, 2024

update

f1d1eb1

wayneguow marked this pull request as ready for review June 27, 2024 02:42

mihailom-db approved these changes Jun 27, 2024

View reviewed changes

cloud-fan reviewed Jul 3, 2024

View reviewed changes

update

f88a1a4

cloud-fan reviewed Jul 5, 2024

View reviewed changes

upgrade

d2941b5

cloud-fan reviewed Jul 5, 2024

View reviewed changes

cloud-fan approved these changes Jul 5, 2024

View reviewed changes

add comments

90111ec

cloud-fan closed this in f1eca90 Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48719][SQL] Fix the calculation bug of `RegrSlope` & `RegrIntercept` when the first parameter is null #47105

[SPARK-48719][SQL] Fix the calculation bug of `RegrSlope` & `RegrIntercept` when the first parameter is null #47105

wayneguow commented Jun 26, 2024 •

edited

Loading

wayneguow Jun 26, 2024

wayneguow commented Jun 27, 2024

wayneguow commented Jun 28, 2024

LuciferYang commented Jul 3, 2024

cloud-fan Jul 3, 2024

wayneguow Jul 3, 2024

cloud-fan Jul 4, 2024

wayneguow Jul 4, 2024

wayneguow Jul 4, 2024

wayneguow Jul 4, 2024

cloud-fan Jul 5, 2024

cloud-fan Jul 5, 2024

cloud-fan Jul 5, 2024 •

edited

Loading

wayneguow Jul 5, 2024

cloud-fan Jul 5, 2024

cloud-fan Jul 5, 2024

cloud-fan commented Jul 5, 2024 •

edited

Loading

cloud-fan commented Jul 5, 2024

wayneguow commented Jul 5, 2024

[SPARK-48719][SQL] Fix the calculation bug of RegrSlope & RegrIntercept when the first parameter is null #47105

[SPARK-48719][SQL] Fix the calculation bug of RegrSlope & RegrIntercept when the first parameter is null #47105

Conversation

wayneguow commented Jun 26, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Choose a reason for hiding this comment

wayneguow commented Jun 27, 2024

wayneguow commented Jun 28, 2024

LuciferYang commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jul 5, 2024 • edited Loading

cloud-fan commented Jul 5, 2024

wayneguow commented Jul 5, 2024

[SPARK-48719][SQL] Fix the calculation bug of `RegrSlope` & `RegrIntercept` when the first parameter is null #47105

[SPARK-48719][SQL] Fix the calculation bug of `RegrSlope` & `RegrIntercept` when the first parameter is null #47105

wayneguow commented Jun 26, 2024 •

edited

Loading

cloud-fan Jul 5, 2024 •

edited

Loading

cloud-fan commented Jul 5, 2024 •

edited

Loading