[SPARK-32136][SQL] NormalizeFloatingNumbers should work on null struct #28962

viirya · 2020-07-01T06:01:40Z

What changes were proposed in this pull request?

This patch fixes wrong groupBy result if the grouping key is a null-value struct.

Why are the changes needed?

NormalizeFloatingNumbers reconstructs a struct if input expression is StructType. If the input struct is null, it will reconstruct a struct with null-value fields, instead of null.

Does this PR introduce any user-facing change?

Yes, fixing incorrect groupBy result.

How was this patch tested?

Unit test.

viirya · 2020-07-01T06:03:11Z

cc @cloud-fan @HyukjinKwon

dongjoon-hyun · 2020-07-01T06:15:00Z

Thank you so much, @viirya .

cloud-fan

good catch and thanks for the quick fix!

dongjoon-hyun · 2020-07-01T06:48:00Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NormalizeFloatingNumbers.scala

@@ -123,7 +123,8 @@ object NormalizeFloatingNumbers extends Rule[LogicalPlan] {
      val fields = expr.dataType.asInstanceOf[StructType].fields.indices.map { i =>
        normalize(GetStructField(expr, i))
      }
-      CreateStruct(fields)
+      val struct = CreateStruct(fields)
+      If(IsNull(expr), Literal(null, struct.dataType), struct)


I'm just wondering if we need a new Literal here. Maybe, can we simple just put expr?

- val struct = CreateStruct(fields) - If(IsNull(expr), Literal(null, struct.dataType), struct) + If(IsNull(expr), expr, CreateStruct(fields))

The AS-IS also looks good to me, too.

literal is better here to simplify the expression tree, and probably has better perf.

SparkQA · 2020-07-01T11:51:27Z

Test build #124746 has finished for PR 28962 at commit 41a318e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-01T11:52:49Z

retest this please

cloud-fan · 2020-07-01T11:58:08Z

The test failure is legitimate, can you take a look? Seems we need to skip the NormalizeFloatingNumbers rule if the input expression contains KnownFloatingPointNormalized

SparkQA · 2020-07-01T17:14:40Z

Test build #124799 has finished for PR 28962 at commit d0586c9.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2020-07-01T17:18:58Z

retest this please

cloud-fan · 2020-07-01T17:27:13Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NormalizeFloatingNumbers.scala

@@ -123,7 +123,8 @@ object NormalizeFloatingNumbers extends Rule[LogicalPlan] {
      val fields = expr.dataType.asInstanceOf[StructType].fields.indices.map { i =>
        normalize(GetStructField(expr, i))
      }
-      CreateStruct(fields)


Do you know why this works before?

CreateStruct re-creates the struct when NormalizeFloatingNumbers is run. The original expr is not used. But now we wrap expr in IsNull.

ah, got it!

This reminds me that we can optimize the code a bit: Instead of only matching CreateNamedStruct, we can have a whitelist to apply the normalization to the children. For example, If(..., CreateNamedStruct(...)), we should apply normalization to the inner CreateNamedStruct.

We can do this later.

For If(..., CreateNamedStruct(...)), it is struct type so already covered by the StructType case. So the normalization is already applied to the inner CreateNamedStruct.

yea I know, but I mean we can do better. We can directly apply the normalization to the children in If(..., CreateNamedStruct(children)), instead of treating the outer If as a black box and apply normalization to it.

SparkQA · 2020-07-01T18:58:06Z

Test build #124771 has finished for PR 28962 at commit 41a318e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-07-01T21:13:55Z

It seems that we hit another relevant failure.

[info] - NaN and -0.0 in join keys *** FAILED *** (1 second, 385 milliseconds)
[info]   org.apache.spark.sql.catalyst.errors.package$TreeNodeException:
Once strategy's idempotence is broken for batch NormalizeFloatingNumbers

viirya · 2020-07-01T21:17:56Z

No, it was triggered before the last commit.

dongjoon-hyun · 2020-07-01T21:59:20Z

Oh, my bad. Got it~

SparkQA · 2020-07-01T23:28:29Z

Test build #124803 has finished for PR 28962 at commit d0586c9.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-07-01T23:36:52Z

PySpark failure looks irrelevant.

FAIL: test_train_prediction (pyspark.mllib.tests.test_streaming_algorithms.StreamingLinearRegressionWithTests)
Test that error on test data improves as model is trained.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jenkins/workspace/SparkPullRequestBuilder@3/python/pyspark/mllib/tests/test_streaming_algorithms.py", line 466, in test_train_prediction
    eventually(condition, timeout=180.0)
  File "/home/jenkins/workspace/SparkPullRequestBuilder@3/python/pyspark/testing/utils.py", line 81, in eventually
    lastValue = condition()
  File "/home/jenkins/workspace/SparkPullRequestBuilder@3/python/pyspark/mllib/tests/test_streaming_algorithms.py", line 461, in condition
    self.assertGreater(errors[1] - errors[-1], 2)
AssertionError: 1.672640157855923 not greater than 2

viirya · 2020-07-01T23:37:47Z

retest this please...

SparkQA · 2020-07-02T04:52:06Z

Test build #124829 has finished for PR 28962 at commit d0586c9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-02T04:56:32Z

Merged to master and branch-3.0.

### What changes were proposed in this pull request? This patch fixes wrong groupBy result if the grouping key is a null-value struct. ### Why are the changes needed? `NormalizeFloatingNumbers` reconstructs a struct if input expression is StructType. If the input struct is null, it will reconstruct a struct with null-value fields, instead of null. ### Does this PR introduce _any_ user-facing change? Yes, fixing incorrect groupBy result. ### How was this patch tested? Unit test. Closes #28962 from viirya/SPARK-32136. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit 3f7780d) Signed-off-by: HyukjinKwon <gurwls223@apache.org>

viirya · 2020-07-02T05:04:25Z

Thanks all!

…seWhen/Coalesce child expressions ### What changes were proposed in this pull request? This patch proposes to let `NormalizeFloatingNumbers` rule directly normalizes on certain children expressions. It could simplify expression tree. ### Why are the changes needed? Currently NormalizeFloatingNumbers rule treats some expressions as black box but we can optimize it a bit by normalizing directly the inner children expressions. Also see #28962 (comment). ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Unit tests. Closes #29061 from viirya/SPARK-32258. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

NormalizeFloatingNumbers should work on null struct.

41a318e

probot-autolabeler bot added the SQL label Jul 1, 2020

cloud-fan approved these changes Jul 1, 2020

View reviewed changes

HyukjinKwon approved these changes Jul 1, 2020

View reviewed changes

dongjoon-hyun reviewed Jul 1, 2020

View reviewed changes

dongjoon-hyun approved these changes Jul 1, 2020

View reviewed changes

maropu approved these changes Jul 1, 2020

View reviewed changes

Fix test.

d0586c9

cloud-fan reviewed Jul 1, 2020

View reviewed changes

HyukjinKwon closed this in 3f7780d Jul 2, 2020

viirya mentioned this pull request Jul 9, 2020

[SPARK-32258][SQL] NormalizeFloatingNumbers directly normalizes IF/CaseWhen/Coalesce child expressions #29061

Closed

viirya deleted the SPARK-32136 branch December 27, 2023 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-32136][SQL] NormalizeFloatingNumbers should work on null struct #28962

[SPARK-32136][SQL] NormalizeFloatingNumbers should work on null struct #28962

viirya commented Jul 1, 2020

viirya commented Jul 1, 2020

dongjoon-hyun commented Jul 1, 2020

cloud-fan left a comment

dongjoon-hyun Jul 1, 2020

dongjoon-hyun Jul 1, 2020

cloud-fan Jul 1, 2020

SparkQA commented Jul 1, 2020

HyukjinKwon commented Jul 1, 2020

cloud-fan commented Jul 1, 2020

SparkQA commented Jul 1, 2020

viirya commented Jul 1, 2020

cloud-fan Jul 1, 2020

viirya Jul 1, 2020

cloud-fan Jul 1, 2020

viirya Jul 1, 2020

viirya Jul 3, 2020

cloud-fan Jul 6, 2020

SparkQA commented Jul 1, 2020

dongjoon-hyun commented Jul 1, 2020

viirya commented Jul 1, 2020

dongjoon-hyun commented Jul 1, 2020

SparkQA commented Jul 1, 2020

dongjoon-hyun commented Jul 1, 2020

viirya commented Jul 1, 2020

SparkQA commented Jul 2, 2020

HyukjinKwon commented Jul 2, 2020

viirya commented Jul 2, 2020

[SPARK-32136][SQL] NormalizeFloatingNumbers should work on null struct #28962

[SPARK-32136][SQL] NormalizeFloatingNumbers should work on null struct #28962

Conversation

viirya commented Jul 1, 2020

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?

viirya commented Jul 1, 2020

dongjoon-hyun commented Jul 1, 2020

cloud-fan left a 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

SparkQA commented Jul 1, 2020

HyukjinKwon commented Jul 1, 2020

cloud-fan commented Jul 1, 2020

SparkQA commented Jul 1, 2020

viirya commented Jul 1, 2020

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

SparkQA commented Jul 1, 2020

dongjoon-hyun commented Jul 1, 2020

viirya commented Jul 1, 2020

dongjoon-hyun commented Jul 1, 2020

SparkQA commented Jul 1, 2020

dongjoon-hyun commented Jul 1, 2020

viirya commented Jul 1, 2020

SparkQA commented Jul 2, 2020

HyukjinKwon commented Jul 2, 2020

viirya commented Jul 2, 2020