anchovYu commented on code in PR #41864:
URL: https://github.com/apache/spark/pull/41864#discussion_r1258881422
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -897,7 +895,20 @@ object FunctionRegistry {
val info = FunctionRegistryBase.expressionInfo[T](name, since)
val funcBuilder = (expressions: Seq[Expression]) => {
assert(expressions.forall(_.resolved), "function arguments must be
resolved.")
- val expr = builder.build(name, expressions)
+ val rearrangedExpressions =
+ if (builder.functionSignatures != Nil) {
Review Comment:
After making it a `Option` following Daniel's comment, we will use
`isDefined` here
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -979,9 +990,7 @@ object TableFunctionRegistry {
: (String, (ExpressionInfo, TableFunctionBuilder)) = {
val (info, builder) = FunctionRegistryBase.build[T](name, since = None)
val newBuilder = (expressions: Seq[Expression]) => {
- val rearrangedExpressions =
- SupportsNamedArguments.getRearrangedExpressions[T](expressions, name)
- val generator = builder(rearrangedExpressions)
+ val generator = builder(expressions)
Review Comment:
What's the plan to support these for named arguments?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/SupportsNamedArguments.scala:
##########
@@ -71,73 +69,17 @@ abstract class SupportsNamedArguments {
}
object SupportsNamedArguments {
-
- /**
- * Given a generic type, we check if the companion object of said type
exists.
- * If that object extends the trait [[SupportsNamedArguments]], then we
rearrange
- * the expressions in the order specified by the object.
- *
- * It is here we resubstitute [[Unevaluable]] [[NamedArgumentExpression]]s
with
- * normal expressions. This method will produce an positional argument list
which
- * is equivalent to the original argumnet list, except the expressions are
now
- * fit for consumption by [[ResolveFunctions]]
- *
- * @param expressions The list of positional and named argument expressions
- * @tparam T The actual expression class.
- * @return positional argument list
- */
- final def getRearrangedExpressions[T : ClassTag](
- expressions: Seq[Expression], functionName: String): Seq[Expression] = {
-
- if (!expressions.exists(_.isInstanceOf[NamedArgumentExpression])) {
- return expressions
- }
-
- import scala.reflect.runtime.currentMirror
-
- // This code heavily utilizes Scala reflection which is unfamiliar to most
developers.
- // Here are the steps of this function:
- // 1. Obtain the module symbol for the companion object of the function
expression.
- // 2. Obtain the module class symbol that represents the companion object.
- // 3. Check if the base classes of the module class symbol contains
SupportsNamedArguments.
- // This checks if the companion object is an implementor of
SupportsNamedArguments.
- // 4. Check if the module class symbol is a top level object. Reflection
is unable to
- // obtain a companion object instance if it is member of some enclosing
class unless
- // instance of said enclosing class is provided which we do not have.
- // 5. Use reflection to obtain instance of companion object and perform
immediate cast to
- // SupportsNamedArguments as it is already verified the cast is safe.
- // 6. Obtain function signature and rearrange expression according to the
given signature.
- val runtimeClass = scala.reflect.classTag[T].runtimeClass
- val targetModuleSymbol = currentMirror.classSymbol(runtimeClass).companion
- val parentClass =
scala.reflect.classTag[SupportsNamedArguments].runtimeClass
- val parentSymbol = currentMirror.classSymbol(parentClass)
-
- if(targetModuleSymbol == scala.reflect.runtime.universe.NoSymbol) {
- throw QueryCompilationErrors.namedArgumentsNotSupported(functionName)
- }
-
- val moduleClassSymbol = targetModuleSymbol.asModule.moduleClass.asClass
- if (!moduleClassSymbol.baseClasses.contains(parentSymbol)) {
- throw QueryCompilationErrors.namedArgumentsNotSupported(functionName)
- }
- if (currentMirror.runtimeClass(moduleClassSymbol).getEnclosingClass !=
null) {
- throw
QueryCompilationErrors.cannotObtainCompanionObjectInstance(functionName)
- }
- val instance = currentMirror.reflectModule(targetModuleSymbol.asModule)
- .instance.asInstanceOf[SupportsNamedArguments]
- if (instance.functionSignatures.size != 1) {
- throw QueryCompilationErrors.multipleFunctionSignatures(
- functionName, instance.functionSignatures)
- }
- instance.rearrange(instance.functionSignatures.head, expressions,
functionName)
- }
-
final def defaultRearrange(functionSignature: FunctionSignature,
args: Seq[Expression],
functionName: String): Seq[Expression] = {
val parameters: Seq[NamedArgument] = functionSignature.parameters
val firstNamedArgIdx: Int =
args.indexWhere(_.isInstanceOf[NamedArgumentExpression])
- val (positionalArgs, namedArgs) = args.splitAt(firstNamedArgIdx)
+ val (positionalArgs, namedArgs) =
+ if (firstNamedArgIdx == -1) {
Review Comment:
If it can't find first named argument, does it just return the original arg
list without reordering? If so you can return early.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/maskExpressions.scala:
##########
@@ -344,4 +346,13 @@ object Mask extends SupportsNamedArguments {
strArg, upperCharArg, lowerCharArg, digitCharArg, otherCharArg))
Seq(functionSignature)
}
+
+ override def build(funcName: String, expressions: Seq[Expression]):
Expression = {
+ if (expressions.length < 1 || expressions.length > 5) {
+ throw QueryCompilationErrors.wrongNumArgsError(
+ funcName, Seq(1, 2, 3, 4, 5), expressions.length)
+ }
+ new Mask(expressions(0), expressions(1), expressions(2), expressions(3),
expressions(4))
Review Comment:
The expression builder way shrifts the responsibility to find a 'right'
constructor (constructor with the right number of arguments) from the
`FunctionRegistryBase.build` that uses reflection, to the expression builder
itself. I'm not sure if this is a preferred way @cloud-fan - to really make
this `mask` function work, we will need to write argument number matching in
the `build` function of the expression builder, e.g. `if (expressions.size ==
2) .. `.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/maskExpressions.scala:
##########
@@ -344,4 +346,13 @@ object Mask extends SupportsNamedArguments {
strArg, upperCharArg, lowerCharArg, digitCharArg, otherCharArg))
Seq(functionSignature)
}
+
+ override def build(funcName: String, expressions: Seq[Expression]):
Expression = {
+ if (expressions.length < 1 || expressions.length > 5) {
+ throw QueryCompilationErrors.wrongNumArgsError(
+ funcName, Seq(1, 2, 3, 4, 5), expressions.length)
+ }
+ new Mask(expressions(0), expressions(1), expressions(2), expressions(3),
expressions(4))
Review Comment:
What happen when there is no named argument but users provide only 3
arguments?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/maskExpressions.scala:
##########
@@ -344,4 +346,13 @@ object Mask extends SupportsNamedArguments {
strArg, upperCharArg, lowerCharArg, digitCharArg, otherCharArg))
Seq(functionSignature)
}
+
+ override def build(funcName: String, expressions: Seq[Expression]):
Expression = {
+ if (expressions.length < 1 || expressions.length > 5) {
+ throw QueryCompilationErrors.wrongNumArgsError(
+ funcName, Seq(1, 2, 3, 4, 5), expressions.length)
+ }
+ new Mask(expressions(0), expressions(1), expressions(2), expressions(3),
expressions(4))
Review Comment:
We can't affect the current usage without named arguments. e.g. we can't
reject these cases. It will become a breaking change.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
