You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by ma...@apache.org on 2026/10/16 08:18:05 UTC
(gluten) branch main updated: [CORE] Keep struct field names in udfLibraryPaths UDF signatures (#13274)
This is an automated email from the ASF dual-hosted git repository.
marin-ma pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gluten.git
The following commit(s) were added to refs/heads/main by this push:
new 822c6da2bb [CORE] Keep struct field names in udfLibraryPaths UDF signatures (#13274)
822c6da2bb is described below
commit 822c6da2bbc53e285c55e4c55a6afe2d44017b35
Author: Pedrum Jalali <pe...@gmail.com>
AuthorDate: Fri Oct 16 01:17:59 2026 -0700
[CORE] Keep struct field names in udfLibraryPaths UDF signatures (#13274)
---
.../apache/gluten/expression/VeloxUdfSuite.scala | 27 +++++++
cpp/velox/substrait/VeloxToSubstraitType.cc | 7 +-
cpp/velox/tests/VeloxToSubstraitTypeTest.cc | 24 ++++++
cpp/velox/udf/examples/MyUDF.cc | 88 ++++++++++++++++++++++
.../apache/gluten/expression/ConverterUtils.scala | 15 +++-
.../gluten/expression/ConverterUtilsSuite.scala | 41 +++++++++-
6 files changed, 195 insertions(+), 7 deletions(-)
diff --git a/backends-velox/src/test/scala/org/apache/gluten/expression/VeloxUdfSuite.scala b/backends-velox/src/test/scala/org/apache/gluten/expression/VeloxUdfSuite.scala
index 493e1c79a7..aba3044e4e 100644
--- a/backends-velox/src/test/scala/org/apache/gluten/expression/VeloxUdfSuite.scala
+++ b/backends-velox/src/test/scala/org/apache/gluten/expression/VeloxUdfSuite.scala
@@ -320,6 +320,33 @@ abstract class VeloxUdfSuite extends GlutenQueryTest with SQLHelper {
assert(e.getMessage.contains("myudf_plus_one"))
}
}
+
+ test("native udf with a named row argument is callable") {
+ // given
+ val query = "SELECT myudf_sum_row(named_struct('a', col1, 'b', col2)) " +
+ "FROM VALUES (1L, 2L), (3L, 4L) AS t(col1, col2)"
+ val expected = Seq(Row(3L), Row(7L))
+
+ // when
+ val df = spark.sql(query)
+
+ // then
+ checkGlutenPlan[ProjectExecTransformer](df)
+ checkAnswer(df, expected)
+ }
+
+ test("native udf returning a named row exposes its field names") {
+ // given
+ val query = "SELECT myudf_make_row(col1).b FROM VALUES (1L), (2L) AS t(col1)"
+ val expected = Seq(Row(2L), Row(3L))
+
+ // when
+ val df = spark.sql(query)
+
+ // then
+ checkGlutenPlan[ProjectExecTransformer](df)
+ checkAnswer(df, expected)
+ }
}
@UDFTest
diff --git a/cpp/velox/substrait/VeloxToSubstraitType.cc b/cpp/velox/substrait/VeloxToSubstraitType.cc
index 90dbc58c2b..c546b4ec79 100644
--- a/cpp/velox/substrait/VeloxToSubstraitType.cc
+++ b/cpp/velox/substrait/VeloxToSubstraitType.cc
@@ -122,9 +122,10 @@ const ::substrait::Type& VeloxToSubstraitTypeConvertor::toSubstraitType(
case velox::TypeKind::ROW: {
::substrait::Type_Struct* substraitStruct =
google::protobuf::Arena::CreateMessage<::substrait::Type_Struct>(&arena);
- for (const auto& child : type->asRow().children()) {
- substraitStruct->set_nullability(::substrait::Type_Nullability_NULLABILITY_NULLABLE);
- substraitStruct->add_types()->MergeFrom(toSubstraitType(arena, child));
+ const auto& rowType = type->asRow();
+ for (uint32_t i = 0; i < rowType.size(); ++i) {
+ substraitStruct->add_types()->MergeFrom(toSubstraitType(arena, rowType.childAt(i)));
+ substraitStruct->add_names(rowType.nameOf(i));
}
substraitStruct->set_nullability(::substrait::Type_Nullability_NULLABILITY_NULLABLE);
substraitType->set_allocated_struct_(substraitStruct);
diff --git a/cpp/velox/tests/VeloxToSubstraitTypeTest.cc b/cpp/velox/tests/VeloxToSubstraitTypeTest.cc
index 9f47770be9..52042bd70d 100644
--- a/cpp/velox/tests/VeloxToSubstraitTypeTest.cc
+++ b/cpp/velox/tests/VeloxToSubstraitTypeTest.cc
@@ -71,4 +71,28 @@ TEST_F(VeloxToSubstraitTypeTest, timestampUtc) {
ASSERT_TRUE(SubstraitParser::parseType(substraitType)->equivalent(*TIMESTAMP_UTC()));
}
+TEST_F(VeloxToSubstraitTypeTest, rowFieldNames) {
+ // given
+ google::protobuf::Arena arena;
+ const auto expected = ROW({"a", "b"}, {BIGINT(), ROW({"x", "y"}, {BOOLEAN(), VARCHAR()})});
+
+ // when
+ const auto actual = SubstraitParser::parseType(typeConvertor_->toSubstraitType(arena, expected));
+
+ // then
+ ASSERT_TRUE(*actual == *expected) << "Expected: " << expected->toString() << ", but got: " << actual->toString();
+}
+
+TEST_F(VeloxToSubstraitTypeTest, rowFieldNamesInContainers) {
+ // given
+ google::protobuf::Arena arena;
+ const auto expected = ROW({"arr", "m"}, {ARRAY(ROW({"elem"}, {VARCHAR()})), MAP(VARCHAR(), ROW({"v"}, {DOUBLE()}))});
+
+ // when
+ const auto actual = SubstraitParser::parseType(typeConvertor_->toSubstraitType(arena, expected));
+
+ // then
+ ASSERT_TRUE(*actual == *expected) << "Expected: " << expected->toString() << ", but got: " << actual->toString();
+}
+
} // namespace gluten
diff --git a/cpp/velox/udf/examples/MyUDF.cc b/cpp/velox/udf/examples/MyUDF.cc
index 687cdd2d3b..84f8c9f5e6 100644
--- a/cpp/velox/udf/examples/MyUDF.cc
+++ b/cpp/velox/udf/examples/MyUDF.cc
@@ -31,6 +31,7 @@ static const char* kInteger = "int";
static const char* kBigInt = "bigint";
static const char* kDate = "date";
static const char* kVarChar = "varchar";
+static const char* kStructAB = "struct<a:bigint,b:bigint>";
namespace hivestringstring {
@@ -108,6 +109,91 @@ class MyUdfPlusOneRegisterer final : public gluten::UdfRegisterer {
} // namespace myudfplusone
+namespace myudfsumrow {
+
+template <typename T>
+struct MyUdfSumRowFunction {
+ VELOX_DEFINE_FUNCTION_TYPES(T);
+
+ FOLLY_ALWAYS_INLINE bool callNullable(int64_t& result, const arg_type<Row<int64_t, int64_t>>* row) {
+ if (row == nullptr) {
+ return false;
+ }
+ const auto a = row->template at<0>();
+ const auto b = row->template at<1>();
+ if (!a.has_value() || !b.has_value()) {
+ return false;
+ }
+ result = a.value() + b.value();
+ return true;
+ }
+};
+
+// name: myudf_sum_row
+// signatures:
+// struct<a:bigint,b:bigint> -> bigint
+// type: SimpleFunction
+class MyUdfSumRowRegisterer final : public gluten::UdfRegisterer {
+ public:
+ int getNumUdf() override {
+ return 1;
+ }
+
+ void populateUdfEntries(int& index, gluten::UdfEntry* udfEntries) override {
+ udfEntries[index++] = {name_.c_str(), kBigInt, 1, arg_, false, false};
+ }
+
+ void registerSignatures() override {
+ facebook::velox::registerFunction<MyUdfSumRowFunction, int64_t, Row<int64_t, int64_t>>({name_});
+ }
+
+ private:
+ const std::string name_ = "myudf_sum_row";
+ const char* arg_[1] = {kStructAB};
+};
+
+} // namespace myudfsumrow
+
+namespace myudfmakerow {
+
+template <typename T>
+struct MyUdfMakeRowFunction {
+ VELOX_DEFINE_FUNCTION_TYPES(T);
+
+ FOLLY_ALWAYS_INLINE bool callNullable(out_type<Row<int64_t, int64_t>>& result, const int64_t* a) {
+ if (a == nullptr) {
+ return false;
+ }
+ result = std::make_tuple(*a, *a + 1);
+ return true;
+ }
+};
+
+// name: myudf_make_row
+// signatures:
+// bigint -> struct<a:bigint,b:bigint>
+// type: SimpleFunction
+class MyUdfMakeRowRegisterer final : public gluten::UdfRegisterer {
+ public:
+ int getNumUdf() override {
+ return 1;
+ }
+
+ void populateUdfEntries(int& index, gluten::UdfEntry* udfEntries) override {
+ udfEntries[index++] = {name_.c_str(), kStructAB, 1, arg_, false, false};
+ }
+
+ void registerSignatures() override {
+ facebook::velox::registerFunction<MyUdfMakeRowFunction, Row<int64_t, int64_t>, int64_t>({name_});
+ }
+
+ private:
+ const std::string name_ = "myudf_make_row";
+ const char* arg_[1] = {kBigInt};
+};
+
+} // namespace myudfmakerow
+
std::vector<std::shared_ptr<gluten::UdfRegisterer>>& globalRegisters() {
static std::vector<std::shared_ptr<gluten::UdfRegisterer>> registerers;
return registerers;
@@ -121,6 +207,8 @@ void setupRegisterers() {
auto& registerers = globalRegisters();
registerers.push_back(std::make_shared<hivestringstring::HiveStringStringRegisterer>());
registerers.push_back(std::make_shared<myudfplusone::MyUdfPlusOneRegisterer>());
+ registerers.push_back(std::make_shared<myudfsumrow::MyUdfSumRowRegisterer>());
+ registerers.push_back(std::make_shared<myudfmakerow::MyUdfMakeRowRegisterer>());
inited = true;
}
} // namespace
diff --git a/gluten-substrait/src/main/scala/org/apache/gluten/expression/ConverterUtils.scala b/gluten-substrait/src/main/scala/org/apache/gluten/expression/ConverterUtils.scala
index cf4803e37b..3fc420ed55 100644
--- a/gluten-substrait/src/main/scala/org/apache/gluten/expression/ConverterUtils.scala
+++ b/gluten-substrait/src/main/scala/org/apache/gluten/expression/ConverterUtils.scala
@@ -189,10 +189,10 @@ object ConverterUtils extends Logging {
(DecimalType(precision, scale), isNullable(decimal.getNullability))
case Type.KindCase.STRUCT =>
val struct_ = substraitType.getStruct
- val fields = struct_.getTypesList.asScala.map {
- typ =>
+ val fields = struct_.getTypesList.asScala.zip(structFieldNames(struct_)).map {
+ case (typ, name) =>
val (field, nullable) = parseFromSubstraitType(typ)
- StructField("", field, nullable)
+ StructField(name, field, nullable)
}
(StructType(fields.toSeq), isNullable(substraitType.getStruct.getNullability))
case Type.KindCase.LIST =>
@@ -213,6 +213,15 @@ object ConverterUtils extends Logging {
}
}
+ // Not every producer sets `names`; without one name per field, the fields stay unnamed.
+ private def structFieldNames(substraitStruct: Type.Struct): Seq[String] = {
+ if (substraitStruct.getNamesCount == substraitStruct.getTypesCount) {
+ substraitStruct.getNamesList.asScala.toSeq
+ } else {
+ Seq.fill(substraitStruct.getTypesCount)("")
+ }
+ }
+
def getTypeNode(datatype: DataType, nullable: Boolean): TypeNode = {
datatype match {
case BooleanType =>
diff --git a/gluten-substrait/src/test/scala/org/apache/gluten/expression/ConverterUtilsSuite.scala b/gluten-substrait/src/test/scala/org/apache/gluten/expression/ConverterUtilsSuite.scala
index bfa3b0ea1b..b20bb1bcf8 100644
--- a/gluten-substrait/src/test/scala/org/apache/gluten/expression/ConverterUtilsSuite.scala
+++ b/gluten-substrait/src/test/scala/org/apache/gluten/expression/ConverterUtilsSuite.scala
@@ -17,11 +17,13 @@
package org.apache.gluten.expression
import org.apache.spark.sql.catalyst.expressions.AttributeReference
-import org.apache.spark.sql.types.{DataType, IntegerType, TimestampType}
+import org.apache.spark.sql.types.{DataType, IntegerType, LongType, StructField, StructType, TimestampType}
import io.substrait.proto.Type
import org.scalatest.funsuite.AnyFunSuiteLike
+import scala.collection.JavaConverters._
+
/**
* Guards the Substrait 0.98 temporal type migration: Spark's TimestampType maps to
* `PrecisionTimestampTZ` and TimestampNTZType maps to `PrecisionTimestamp`, both at microsecond
@@ -75,4 +77,41 @@ class ConverterUtilsSuite extends AnyFunSuiteLike {
val named = AttributeReference("B", IntegerType)()
assert(ConverterUtils.genColumnNameWithExprId(named) === s"b#${named.exprId.id}")
}
+
+ test("struct field names are kept when the producer sets them") {
+ // given
+ val proto = structOfTwoLongs(Seq("a", "b"))
+ val expected = StructType(Seq(StructField("a", LongType), StructField("b", LongType)))
+
+ // when
+ val (actual, _) = ConverterUtils.parseFromSubstraitType(proto)
+
+ // then
+ assert(actual === expected)
+ }
+
+ test("struct fields stay unnamed when the producer sets no names") {
+ // given
+ val proto = structOfTwoLongs(Seq.empty)
+ val expected = StructType(Seq(StructField("", LongType), StructField("", LongType)))
+
+ // when
+ val (actual, _) = ConverterUtils.parseFromSubstraitType(proto)
+
+ // then
+ assert(actual === expected)
+ }
+
+ private def structOfTwoLongs(names: Seq[String]): Type = {
+ val i64 = Type
+ .newBuilder()
+ .setI64(Type.I64.newBuilder().setNullability(Type.Nullability.NULLABILITY_NULLABLE))
+ val struct_ = Type.Struct
+ .newBuilder()
+ .setNullability(Type.Nullability.NULLABILITY_NULLABLE)
+ .addTypes(i64)
+ .addTypes(i64)
+ .addAllNames(names.asJava)
+ Type.newBuilder().setStruct(struct_).build()
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org