Skip to content

Conversation

@rednaxelafx
Copy link
Contributor

What changes were proposed in this pull request?

Constant fold deterministic Scala UDFs with foldable arguments, conservatively.
ScalaUDFs that meet all following criteria are subject to constant folding in this PR:

  • deterministic
  • all arguments are foldable
  • does not throw an exception when evaluating the UDF for constant folding

Why are the changes needed?

This is an optimization that enables more constant folding, improving the performance of a special case of Scala UDFs.

Catalyst already implements constant folding of expressions in a few places, through the pattern if (e.foldable) e.eval(EmptyRow). One of the optimizer rules that specifically performs constant folding is ConstantFolding.

ScalaUDF does not override foldable, so it isn't subject to constant folding right now.

To enable constant folding of ScalaUDFs, it's tempting to make it override foldable:

  override lazy val foldable: Boolean = deterministic && children.forall(_.foldable)

But ScalaUDFs are declared as deterministic by default, so it's possible for users to mis-declare a potentially exception-throwing UDF as deterministic, so for callers of the "constant folding" code pattern that do not expect an exception, overriding ScalaUDF.foldable in general may have a wide impact.

Instead, this PR tackles the problem in a conservative way, where the constant folding of ScalaUDFs is only implemented in the ConstantFolding rule, with special handling to catch non-fatal exceptions when evaluating a Scala UDF during constant folding and skip folding such UDFs.

Does this PR introduce any user-facing change?

No.

Caveat: this PR assumes certain semantics of Expression.deterministic. Although it guards against potentially exception-throwing Scala UDFs being mis-declared as deterministic, it cannot guard against other kinds of mis-declarations, e.g.

  • UDFs that are only expected to be invoked on executors;
  • UDFs that return the same value but have other side-effects, such as modifying a static field.
    Users should explicitly mark such UDFs as asNondeterministic.

How was this patch tested?

Added a test case in ConstantFoldingSuite.

…g criteria:

- deterministic
- all arguments are foldable
- does not throw an exception upon evaluation
@rednaxelafx rednaxelafx changed the title [WIP][SPARK-XXXXX][SQL] Constant fold deterministic Scala UDFs with foldable arguments [SPARK-31240][SQL] Constant fold deterministic Scala UDFs with foldable arguments Mar 25, 2020
@SparkQA
Copy link

SparkQA commented Mar 25, 2020

Test build #120295 has finished for PR 28008 at commit aed59d7.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

FYI, there was a similar try at #24593

@maropu
Copy link
Member

maropu commented Mar 25, 2020

I looked over the two jira and I feel both are the same, so I will close the newer one as duplicated and update the PR title. If there is any problem, please re-update them.

@maropu maropu changed the title [SPARK-31240][SQL] Constant fold deterministic Scala UDFs with foldable arguments [SPARK-27692][SQL] Constant fold deterministic Scala UDFs with foldable arguments Mar 25, 2020
@rednaxelafx
Copy link
Contributor Author

Thank you very much for linking this PR to the existing JIRA ticket and its PR!
I vaguely remember this had been implemented before but didn't find it in master and got curious... now I know >_<|||

It's highly likely that this PR won't be good enough to make the bar, but I'd like to hear other folks' opinions.
Thanks.

Basically this is a hurdle that we cannot get past until the APIs are clarified and the semantics of ScalaUDF.deterministic is strong enough for optimizations to depend on:
#24593 (review)

I had the same concerns when sending out this PR, and I'm more than okay to withdraw and close this PR given the current state of the API/semantics.

@rednaxelafx
Copy link
Contributor Author

Closing this PR as I don't think it's going to work given the current API semantics.

@SparkQA
Copy link

SparkQA commented Mar 25, 2020

Test build #120297 has finished for PR 28008 at commit ab15f5c.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants