[SPARK-37957][SQL] Correctly pass deterministic flag for V2 scalar functions #35243

sunchao · 2022-01-18T21:57:32Z

What changes were proposed in this pull request?

Pass isDeterministic flag to ApplyFunctionExpression, Invoke and StaticInvoke when processing V2 scalar functions.

Why are the changes needed?

A V2 scalar function can be declared as non-deterministic. However, currently Spark doesn't pass the flag when converting the V2 function to a catalyst expression, which could lead to incorrect results if being applied with certain optimizations.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added a unit test.

huaxingao · 2022-01-19T00:13:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

    propagateNull: Boolean = true,
-    returnNullable: Boolean = true) extends InvokeLike {
+    returnNullable: Boolean = true,
+    isDeterministic: Boolean = true) extends InvokeLike {


why this defaults to true?

I think in majority cases a function is deterministic, so defaulting to true here. This is similar to how we treat propagateNull and returnNullable here.

+1 for default true.

huaxingao · 2022-01-19T00:15:27Z

sql/core/src/test/java/test/org/apache/spark/sql/connector/catalog/functions/JavaRandomAdd.java

+  }
+}
+
+


super nit: extra blank line

AngersZhuuuu · 2022-01-19T02:22:42Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2FunctionSuite.scala

    }
  }

+  test("SPARK-37957: pass deterministic flag") {


Can we make the test name more clear? pass deterministic flat to what

cloud-fan · 2022-01-19T06:17:53Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala


  override def nullable: Boolean = needNullCheck || returnNullable
  override def children: Seq[Expression] = arguments
+  override lazy val deterministic: Boolean = isDeterministic


we should also check if all the arguments are deterministic.

Oops you are right. Let me fix it.

cloud-fan · 2022-01-19T17:59:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala


  override def nullable: Boolean = needNullCheck || returnNullable
  override def children: Seq[Expression] = arguments
+  override lazy val deterministic: Boolean = isDeterministic && arguments.forall(_.deterministic)


nit: seems we can move this to InvokeLike

I thought about this too, but then we'd have to move isDeterministic there too and then override it in StaticInvoke and Invoke, so doesn't seem we can save much. Plus I think deterministic property is not useful for NewInstance.

…nctions ### What changes were proposed in this pull request? Pass `isDeterministic` flag to `ApplyFunctionExpression`, `Invoke` and `StaticInvoke` when processing V2 scalar functions. ### Why are the changes needed? A V2 scalar function can be declared as non-deterministic. However, currently Spark doesn't pass the flag when converting the V2 function to a catalyst expression, which could lead to incorrect results if being applied with certain optimizations. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added a unit test. Closes #35243 from sunchao/SPARK-37957. Authored-by: Chao Sun <sunchao@apple.com> Signed-off-by: Chao Sun <sunchao@apple.com>

sunchao · 2022-01-19T22:10:16Z

Thanks! merged to master and branch-3.2.

dongjoon-hyun · 2022-01-19T23:58:30Z

Thank you all!

HyukjinKwon · 2022-02-03T03:16:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

    propagateNull: Boolean = true,
-    returnNullable: Boolean = true) extends InvokeLike {
+    returnNullable: Boolean = true,
+    isDeterministic: Boolean = true) extends InvokeLike {


I think this might be controversial to backport .. as

V2 expressions are unstable yet.

It could lead to different results in maintenance version upgrade if a user sets isDeterministic to false

Maybe performance regression if a user sets isDeterministic to false.

We haven't heard an actual complaint from user mailing list.

This makes an API (isDeterministic) working that has never been working in a way (is this a bug fix?)

While I agree with this being merged in 3.3.0, and I don't feel strongly on this in 3.2.X, maybe we can consider reverting this out of branch-3.2 because it has a good and bad thing. If we're worried about this change, we could issue a warning instead when isDeterministic from V2 Scalar Function returns false.

I will leave it to you @sunchao.

I think this is a bug fix. A V2 catalog can return a function that's non-deterministic, while without the fix Spark can treat it as deterministic and apply related optimization rules (e.g., constant folding), which could cause correctness issues.

Since this is already in Spark 3.2.1, I don't see much benefit of reverting it and re-introduce the correctness issue.

+1 on keeping this in 3.2.x. This fixed the correctness issue and we actually intentionally included this fix in 3.2.1 release.

That's fine. I didn't have a strong preference so I am okay with keeping it either 👍 .

…nctions ### What changes were proposed in this pull request? Pass `isDeterministic` flag to `ApplyFunctionExpression`, `Invoke` and `StaticInvoke` when processing V2 scalar functions. ### Why are the changes needed? A V2 scalar function can be declared as non-deterministic. However, currently Spark doesn't pass the flag when converting the V2 function to a catalyst expression, which could lead to incorrect results if being applied with certain optimizations. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added a unit test. Closes apache#35243 from sunchao/SPARK-37957. Authored-by: Chao Sun <sunchao@apple.com> Signed-off-by: Chao Sun <sunchao@apple.com>

…nctions ### What changes were proposed in this pull request? Pass `isDeterministic` flag to `ApplyFunctionExpression`, `Invoke` and `StaticInvoke` when processing V2 scalar functions. ### Why are the changes needed? A V2 scalar function can be declared as non-deterministic. However, currently Spark doesn't pass the flag when converting the V2 function to a catalyst expression, which could lead to incorrect results if being applied with certain optimizations. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added a unit test. Closes apache#35243 from sunchao/SPARK-37957. Authored-by: Chao Sun <sunchao@apple.com> Signed-off-by: Chao Sun <sunchao@apple.com> (cherry picked from commit 3860ac5) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

wip

d2ac376

github-actions bot added the SQL label Jan 18, 2022

sunchao mentioned this pull request Jan 18, 2022

[SPARK-37907][SQL] InvokeLike support ConstantFolding #35207

Closed

retrigger build

0703038

huaxingao reviewed Jan 19, 2022

View reviewed changes

code style

9ac42b7

AngersZhuuuu reviewed Jan 19, 2022

View reviewed changes

cloud-fan reviewed Jan 19, 2022

View reviewed changes

address comments

774ae34

cloud-fan reviewed Jan 19, 2022

View reviewed changes

cloud-fan approved these changes Jan 19, 2022

View reviewed changes

viirya approved these changes Jan 19, 2022

View reviewed changes

sunchao closed this in 9ffca0c Jan 19, 2022

sunchao deleted the SPARK-37957 branch January 19, 2022 22:10

thesamet mentioned this pull request Jan 31, 2022

Compatibility with Spark 3.2.1 typelevel/frameless#605

Closed

HyukjinKwon mentioned this pull request Feb 3, 2022

[SPARK-38077][SQL] Fix binary compatibility issue with isDeterministic flag #35378

Closed

HyukjinKwon reviewed Feb 3, 2022

View reviewed changes

[SPARK-37957][SQL] Correctly pass deterministic flag for V2 scalar functions #35243

[SPARK-37957][SQL] Correctly pass deterministic flag for V2 scalar functions #35243

Uh oh!

Conversation

sunchao commented Jan 18, 2022

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunchao commented Jan 19, 2022

Uh oh!

dongjoon-hyun commented Jan 19, 2022

Uh oh!

HyukjinKwon Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

HyukjinKwon Feb 3, 2022 •

edited

Loading