You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "PHILO-HE (via GitHub)" <gi...@apache.org> on 2025/03/24 07:15:16 UTC
Re: [PR] [GLUTEN-9076][VL] Prioritize offloading supported hive udf in ColumnarPartialProject [incubator-gluten]
PHILO-HE commented on code in PR #9077:
URL: https://github.com/apache/incubator-gluten/pull/9077#discussion_r2009542190
##########
backends-velox/src/main/scala/org/apache/gluten/execution/ColumnarPartialProjectExec.scala:
##########
@@ -275,7 +275,10 @@ object ColumnarPartialProjectExec {
if (expr == null) return false
expr match {
case _: ScalaUDF => true
- case h if HiveUdfUtil.isHiveUdf(h) => true
+ case h
+ if HiveUdfUtil.isHiveUdf(h) &&
+ !VeloxHiveUDFTransformer.isHiveUDFSupportsTransform(h) =>
Review Comment:
Nit: change the function name, e.g., `containsUnsupportedUDF`, to reflect the new code logic.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/ColumnarPartialProjectExec.scala:
##########
@@ -306,7 +309,9 @@ object ColumnarPartialProjectExec {
expr match {
case u: ScalaUDF =>
replaceByAlias(u, replacedAliasUdf)
- case h if HiveUdfUtil.isHiveUdf(h) =>
+ case h
+ if HiveUdfUtil.isHiveUdf(h) &&
Review Comment:
ditto.
##########
backends-velox/src/main/scala/org/apache/spark/sql/hive/VeloxHiveUDFTransformer.scala:
##########
@@ -46,4 +40,20 @@ object VeloxHiveUDFTransformer {
HiveUDFTransformer.genTransformerFromUDFMappings(udfName, expr, attributeSeq)
}
}
+
+ def isHiveUDFSupportsTransform(expr: Expression): Boolean = {
Review Comment:
Nit, naming suggestion:
isSupportedHiveUDF
##########
backends-velox/src/main/scala/org/apache/gluten/execution/ColumnarPartialProjectExec.scala:
##########
@@ -275,7 +275,10 @@ object ColumnarPartialProjectExec {
if (expr == null) return false
expr match {
case _: ScalaUDF => true
- case h if HiveUdfUtil.isHiveUdf(h) => true
+ case h
+ if HiveUdfUtil.isHiveUdf(h) &&
Review Comment:
Just a comment on this existing code. The `isHiveUdf` covers UDAF and UDTF, which seems meaningless here for partial project. Maybe, just use `HiveUDFTransformer.isHiveUDF`?
##########
backends-velox/src/test/scala/org/apache/spark/sql/execution/GlutenHiveUDFSuite.scala:
##########
@@ -200,4 +201,54 @@ class GlutenHiveUDFSuite extends GlutenQueryTest with SQLTestUtils {
checkOperatorMatch[ColumnarPartialProjectExec](df)
}
}
+
+ test("UDFMapping should prioritize over ColumnarPartialProject when both applicable") {
Review Comment:
make it friendly to reader: e.g., "prioritize offloading supported hive udf in ColumnarPartialProject"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org