Skip to content

[SPARK-42051][SQL] Codegen Support for HiveGenericUDF - #39555

Closed
yaooqinn wants to merge 9 commits into
apache:masterfrom
yaooqinn:SPARK-42051
Closed

[SPARK-42051][SQL] Codegen Support for HiveGenericUDF#39555
yaooqinn wants to merge 9 commits into
apache:masterfrom
yaooqinn:SPARK-42051

Conversation

@yaooqinn

@yaooqinnyaooqinn commented Jan 13, 2023•
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

As a subtask of SPARK-42050, this PR adds Codegen Support for HiveGenericUDF

Why are the changes needed?

improve codegen coverage and performance

Does this PR introduce any user-facing change?

no

How was this patch tested?

new UT added

@yaooqinn

yaooqinn commented Jan 13, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

cc @cloud-fan@HyukjinKwon@dongjoon-hyun PTAL, thanks

@yaooqinnyaooqinn self-assigned this Jan 13, 2023
cloud-fan
cloud-fan reviewed Jan 13, 2023
View reviewed changes
Comment threadsql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala Outdated
}

private[hive] case class HiveGenericUDF(
case class HiveGenericUDF(

@cloud-fancloud-fanJan 13, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to rewrite HiveGenericUDF with Invoke? Then we can simply use RuntimeReplaceable

@yaooqinnyaooqinnJan 16, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems quite complicated to handle the hive value mapping and function wrapping

dongjoon-hyun
dongjoon-hyun reviewed Jan 20, 2023
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @sunchao , too

dongjoon-hyun
dongjoon-hyun approved these changes Jan 22, 2023
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. The current approach also looks reasonable to me.

Could you review this once more when you have some time, @cloud-fan and @sunchao ?

@dongjoon-hyun

dongjoon-hyun commented Jan 22, 2023

Copy link
Copy Markdown
Member

Also cc @LuciferYang , too

LuciferYang
LuciferYang reviewed Jan 23, 2023
View reviewed changes
Comment threadsql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala Outdated
Comment threadsql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala Outdated
Comment threadsql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala
Comment threadsql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala
@yaooqinn

yaooqinn commented Jan 31, 2023

Copy link
Copy Markdown
MemberAuthor

Belated Happy new year!
Comments addressed, thank you all.

@yaooqinn

yaooqinn commented Feb 1, 2023

Copy link
Copy Markdown
MemberAuthor

Since the last commit is for adding a test, I will merge this. thanks @dongjoon-hyun@LuciferYang.

Merged to master

@dongjoon-hyun

dongjoon-hyun commented Feb 1, 2023

Copy link
Copy Markdown
Member

Thank you, @yaooqinn and @LuciferYang !

@LuciferYang

LuciferYang commented Feb 1, 2023

Copy link
Copy Markdown
Contributor

late LGTM

@HyukjinKwonHyukjinKwon mentioned this pull request Feb 3, 2023
Closed
@yaooqinn
yaooqinn deleted the SPARK-42051 branch February 3, 2023 07:35
@panbingkunpanbingkun mentioned this pull request Feb 9, 2023
Closed
@andygroveandygrove mentioned this pull request Jul 31, 2023
Closed
andygrove
andygrove reviewed Jul 31, 2023
View reviewed changes
Comment threadsql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala
extends DeferredObject with HiveInspectors {

private val wrapper = wrapperFor(oi, dataType)
private var func: () => Any = _

@andygroveandygroveJul 31, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaooqinn@dongjoon-hyun This change removes deferred evaluation and means it is no longer possible to implement short-circuiting in Hive generic UDFs. I filed https://issues.apache.org/jira/browse/SPARK-44616 for this.

@jackylee-chjackylee-chJul 2, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During the upgrade from Spark 3.3.1 to 3.5.1, we encountered syntax issues with this pr. The problem arose from DeferredObject currently passing a value instead of a function, which prevented users from catching exceptions in GenericUDF, resulting in semantic differences.

Here is an example case we encountered. Originally, the semantics were that str_to_map_udf would throw an exception due to issues with the input string, while merge_map_udf could catch the exception and return a null value. However, currently, any exception encountered by str_to_map_udf will cause the program to fail.

select merge_map_udf(str_to_map_udf(col1), parse_map_udf(col2), map("key", "value")) from table

@cloud-fancloud-fanJul 2, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaooqinn is it easy to fix? If not we should probably revert it as this is not a critical perf improvement.

@yaooqinnyaooqinnJul 2, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being late，my network glitches a lot recently. and thanks for reporting this issue. It’s easy to make a fix

@panbingkunpanbingkunJul 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaooqinn, is this already underway? I tried this on local #47193

@yaooqinnyaooqinnJul 3, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@panbingkun thank you

@jackylee-chjackylee-chJul 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to fix it in #47268 in another way, @yaooqinn would you please take a look?

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

Reviewers

@LuciferYangLuciferYangLuciferYang left review comments
@cloud-fancloud-fancloud-fan left review comments
@panbingkunpanbingkunpanbingkun left review comments
@dongjoon-hyundongjoon-hyundongjoon-hyun approved these changes
+2 more reviewers
@andygroveandygroveandygrove left review comments
@jackylee-chjackylee-chjackylee-ch left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@yaooqinnyaooqinn

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@yaooqinn@dongjoon-hyun@LuciferYang@andygrove@cloud-fan@jackylee-ch@panbingkun