You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/15 15:55:44 UTC
[GitHub] [spark] cloud-fan opened a new pull request #35534: [DO NOT MERGE] Improve RuntimeReplaceable
cloud-fan opened a new pull request #35534:
URL: https://github.com/apache/spark/pull/35534
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809978242
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -388,29 +395,13 @@ trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
* with other databases. For example, we use this to support every, any/some aggregates by rewriting
* them with Min and Max respectively.
*/
-trait UnevaluableAggregate extends DeclarativeAggregate {
-
- override def nullable: Boolean = true
-
- override lazy val aggBufferAttributes =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "aggBufferAttributes", this)
-
- override lazy val initialValues: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "initialValues", this)
-
- override lazy val updateExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "updateExpressions", this)
-
- override lazy val mergeExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "mergeExpressions", this)
-
- override lazy val evaluateExpression: Expression =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "evaluateExpression", this)
+abstract class RuntimeReplaceableAggregate extends AggregateFunction with RuntimeReplaceable {
+ def aggBufferSchema: StructType = throw new IllegalStateException(
+ "RuntimeReplaceableAggregate.aggBufferSchema should not be called")
+ def aggBufferAttributes: Seq[AttributeReference] = throw new IllegalStateException(
Review comment:
These methods have no logic so I just put them together without blank lines. See also `UnresolvedAttribute`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809671431
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
Review comment:
We need it. `inputTypes` doesn't work for `TryAdd`, it needs more complicated type coercion.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809597467
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
Review comment:
Hm, why is it called "type coercion"? From a cursory look, it seems like it reduces the duplication for pretty SQL string.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809154143
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
##########
@@ -405,30 +404,32 @@ case class AesDecrypt(
input: Expression,
key: Expression,
mode: Expression,
- padding: Expression,
- child: Expression)
- extends RuntimeReplaceable {
-
- def this(input: Expression, key: Expression, mode: Expression, padding: Expression) = {
- this(
- input,
- key,
- mode,
- padding,
- StaticInvoke(
- classOf[ExpressionImplUtils],
- BinaryType,
- "aesDecrypt",
- Seq(input, key, mode, padding),
- Seq(BinaryType, BinaryType, StringType, StringType)))
- }
+ padding: Expression)
+ extends RuntimeReplaceable with ImplicitCastInputTypes {
+
+ lazy val replacement: Expression = StaticInvoke(
+ classOf[ExpressionImplUtils],
+ BinaryType,
+ "aesDecrypt",
+ Seq(input, key, mode, padding),
+ inputTypes)
+
def this(input: Expression, key: Expression, mode: Expression) =
this(input, key, mode, Literal("DEFAULT"))
def this(input: Expression, key: Expression) =
this(input, key, Literal("GCM"))
- def exprsReplaced: Seq[Expression] = Seq(input, key)
Review comment:
This is a mistake that we didn't include `mode` and `padding` here.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809202404
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and type coercion behavior. The `RuntimeReplaceable` expression will be
+ // replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
+ // `RuntimeReplaceable` to define the combination. See `ParseToDate` as an example.
+ // We can also inherit the type coercion behavior from the replacement expression, by
+ // extending `RuntimeReplaceableInheritingTypeCoercion`. See `TryAdd` as an example.
Review comment:
@cloud-fan It seems that `TryAdd` inherits the type coercion of `Add`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809708933
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
+ // `RuntimeReplaceable` to define the combination. See `ParseToDate` as an example.
+ // We can also inherit the analysis behavior from the replacement expression, by
+ // mixing `InheritAnalysisRules`. See `TryAdd` as an example.
+ // - Similarly, we can use `RuntimeReplaceableAggregate` to implement new aggregate functions.
Review comment:
Will follow up and take a look (https://github.com/apache/spark/pull/35534#discussion_r809708015)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809608180
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
+ extends RuntimeReplaceable with UnaryLike[Expression] {
Review comment:
@cloud-fan what about this idea? we leverage a self-typed trait that forces inheriting `RuntimeReplaceable` together, and we name it something like `ReplaceChild`, `ChildReplaceable`, `InheritExpressionRules`, ... etc.?e.g.)
```sala
trait ChildReplaceable extends UnaryLike[Expression] { self: RuntimeReplaceable =>
...
```
This will force downstream expressions to implement `RuntimeReplaceable` together, and wonder if this would be easier to follow the codes.
My concerns are:
- `*TypeCoercion` is that it actually can do more than just type coercion. For now, it's type coercion only but we could have more rules on that .. and we would have to change the name again ..
- Having `RuntimeReplaceable` and `RuntimeReplaceableInheritingTypeCoercion` might be less readable
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35534:
URL: https://github.com/apache/spark/pull/35534
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35534:
URL: https://github.com/apache/spark/pull/35534#issuecomment-1043053897
cc @gengliangwang @MaxGekk @HyukjinKwon @beliefer @AngersZhuuuu @xinrong-databricks @viirya
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809626731
##########
File path: sql/core/src/test/resources/sql-functions/sql-expression-schema.md
##########
@@ -182,8 +181,8 @@
| org.apache.spark.sql.catalyst.expressions.MakeDate | make_date | SELECT make_date(2013, 7, 15) | struct<make_date(2013, 7, 15):date> |
| org.apache.spark.sql.catalyst.expressions.MakeInterval | make_interval | SELECT make_interval(100, 11, 1, 1, 12, 30, 01.001001) | struct<make_interval(100, 11, 1, 1, 12, 30, 1.001001):interval> |
| org.apache.spark.sql.catalyst.expressions.MakeTimestamp | make_timestamp | SELECT make_timestamp(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp(2014, 12, 28, 6, 30, 45.887):timestamp> |
-| org.apache.spark.sql.catalyst.expressions.MakeTimestampLTZ | make_timestamp_ltz | SELECT make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887):timestamp> |
-| org.apache.spark.sql.catalyst.expressions.MakeTimestampNTZ | make_timestamp_ntz | SELECT make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887):timestamp_ntz> |
+| org.apache.spark.sql.catalyst.expressions.MakeTimestampLTZExpressionBuilder$ | make_timestamp_ltz | SELECT make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887):timestamp> |
+| org.apache.spark.sql.catalyst.expressions.MakeTimestampNTZExpressionBuilder$ | make_timestamp_ntz | SELECT make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887):timestamp_ntz> |
Review comment:
Although `MakeTimestampNTZExpressionBuilder` looks useful, but seems strange here.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809708015
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -388,29 +395,13 @@ trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
* with other databases. For example, we use this to support every, any/some aggregates by rewriting
* them with Min and Max respectively.
*/
-trait UnevaluableAggregate extends DeclarativeAggregate {
-
- override def nullable: Boolean = true
-
- override lazy val aggBufferAttributes =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "aggBufferAttributes", this)
-
- override lazy val initialValues: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "initialValues", this)
-
- override lazy val updateExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "updateExpressions", this)
-
- override lazy val mergeExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "mergeExpressions", this)
-
- override lazy val evaluateExpression: Expression =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "evaluateExpression", this)
+abstract class RuntimeReplaceableAggregate extends AggregateFunction with RuntimeReplaceable {
Review comment:
Actually I think we can make this as a trait w/ switching `AggregateFunction` to a trait.
```diff
-abstract class AggregateFunction extends Expression {
+trait AggregateFunction extends Expression {
```
this at least compiles fine but I would prefer to do it separately. I will follow up after this got merged.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35534:
URL: https://github.com/apache/spark/pull/35534#issuecomment-1048511829
thanks for the review, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809671064
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and type coercion behavior. The `RuntimeReplaceable` expression will be
+ // replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
+ // `RuntimeReplaceable` to define the combination. See `ParseToDate` as an example.
+ // We can also inherit the type coercion behavior from the replacement expression, by
+ // extending `RuntimeReplaceableInheritingTypeCoercion`. See `TryAdd` as an example.
Review comment:
It inherits the type coercion of its `replacement` expression, which is `TryEval(Add(...))`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809672298
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
##########
@@ -126,8 +126,8 @@ object RaiseError {
""",
since = "2.0.0",
group = "misc_funcs")
-case class AssertTrue(left: Expression, right: Expression, child: Expression)
- extends RuntimeReplaceable {
+case class AssertTrue(left: Expression, right: Expression, replacement: Expression)
+ extends RuntimeReplaceableInheritingTypeCoercion {
Review comment:
See the classdoc of `RuntimeReplaceableInheritingTypeCoercion`. `replacement` must be in the constructor, so that catalyst rules can transform it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809709091
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
+ // `RuntimeReplaceable` to define the combination. See `ParseToDate` as an example.
+ // We can also inherit the analysis behavior from the replacement expression, by
+ // mixing `InheritAnalysisRules`. See `TryAdd` as an example.
+ // - Similarly, we can use `RuntimeReplaceableAggregate` to implement new aggregate functions.
Review comment:
ideally it should, but we don't have a test case yet and this is not tested.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35534:
URL: https://github.com/apache/spark/pull/35534#issuecomment-1043823896
Looks pretty good otherwise.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809679664
##########
File path: sql/core/src/test/resources/sql-functions/sql-expression-schema.md
##########
@@ -182,8 +181,8 @@
| org.apache.spark.sql.catalyst.expressions.MakeDate | make_date | SELECT make_date(2013, 7, 15) | struct<make_date(2013, 7, 15):date> |
| org.apache.spark.sql.catalyst.expressions.MakeInterval | make_interval | SELECT make_interval(100, 11, 1, 1, 12, 30, 01.001001) | struct<make_interval(100, 11, 1, 1, 12, 30, 1.001001):interval> |
| org.apache.spark.sql.catalyst.expressions.MakeTimestamp | make_timestamp | SELECT make_timestamp(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp(2014, 12, 28, 6, 30, 45.887):timestamp> |
-| org.apache.spark.sql.catalyst.expressions.MakeTimestampLTZ | make_timestamp_ltz | SELECT make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887):timestamp> |
-| org.apache.spark.sql.catalyst.expressions.MakeTimestampNTZ | make_timestamp_ntz | SELECT make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887):timestamp_ntz> |
+| org.apache.spark.sql.catalyst.expressions.MakeTimestampLTZExpressionBuilder$ | make_timestamp_ltz | SELECT make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887):timestamp> |
+| org.apache.spark.sql.catalyst.expressions.MakeTimestampNTZExpressionBuilder$ | make_timestamp_ntz | SELECT make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887):timestamp_ntz> |
Review comment:
Yeah.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r810761848
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
Review comment:
Yea I also hit this issue with `MapContainsKey`. Its type coercion behavior is very similar to `ArrayContains` but is slightly different. I just copy-paste the code and modify for `MapContainsKey`.
In the long term, I think we should remove `InheritAnalysisRules` and ask every (runtime replaceable) expression to define its own analysis behaviors. We can provide many traits for common analysis behaviors to reuse code. And some traits can have a few flags to tune part of the behaviors.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809672518
##########
File path: sql/core/src/test/resources/sql-functions/sql-expression-schema.md
##########
@@ -182,8 +181,8 @@
| org.apache.spark.sql.catalyst.expressions.MakeDate | make_date | SELECT make_date(2013, 7, 15) | struct<make_date(2013, 7, 15):date> |
| org.apache.spark.sql.catalyst.expressions.MakeInterval | make_interval | SELECT make_interval(100, 11, 1, 1, 12, 30, 01.001001) | struct<make_interval(100, 11, 1, 1, 12, 30, 1.001001):interval> |
| org.apache.spark.sql.catalyst.expressions.MakeTimestamp | make_timestamp | SELECT make_timestamp(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp(2014, 12, 28, 6, 30, 45.887):timestamp> |
-| org.apache.spark.sql.catalyst.expressions.MakeTimestampLTZ | make_timestamp_ltz | SELECT make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887):timestamp> |
-| org.apache.spark.sql.catalyst.expressions.MakeTimestampNTZ | make_timestamp_ntz | SELECT make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887):timestamp_ntz> |
+| org.apache.spark.sql.catalyst.expressions.MakeTimestampLTZExpressionBuilder$ | make_timestamp_ltz | SELECT make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887):timestamp> |
+| org.apache.spark.sql.catalyst.expressions.MakeTimestampNTZExpressionBuilder$ | make_timestamp_ntz | SELECT make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887):timestamp_ntz> |
Review comment:
This is internal. end-users won't see it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809118029
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
Review comment:
This is the same as the old `RuntimeReplaceable` before this PR.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809708015
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -388,29 +395,13 @@ trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
* with other databases. For example, we use this to support every, any/some aggregates by rewriting
* them with Min and Max respectively.
*/
-trait UnevaluableAggregate extends DeclarativeAggregate {
-
- override def nullable: Boolean = true
-
- override lazy val aggBufferAttributes =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "aggBufferAttributes", this)
-
- override lazy val initialValues: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "initialValues", this)
-
- override lazy val updateExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "updateExpressions", this)
-
- override lazy val mergeExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "mergeExpressions", this)
-
- override lazy val evaluateExpression: Expression =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "evaluateExpression", this)
+abstract class RuntimeReplaceableAggregate extends AggregateFunction with RuntimeReplaceable {
Review comment:
Actually I think we can make this as a trait w/ switching `AggregateFunction` to a trait.
```diff
-abstract class AggregateFunction extends Expression {
+trait AggregateFunction extends Expression {
```
this at least compiles fine but I would prefer to do it separately. I will follow up after this get merged.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -388,29 +395,13 @@ trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
* with other databases. For example, we use this to support every, any/some aggregates by rewriting
* them with Min and Max respectively.
*/
-trait UnevaluableAggregate extends DeclarativeAggregate {
-
- override def nullable: Boolean = true
-
- override lazy val aggBufferAttributes =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "aggBufferAttributes", this)
-
- override lazy val initialValues: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "initialValues", this)
-
- override lazy val updateExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "updateExpressions", this)
-
- override lazy val mergeExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "mergeExpressions", this)
-
- override lazy val evaluateExpression: Expression =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "evaluateExpression", this)
+abstract class RuntimeReplaceableAggregate extends AggregateFunction with RuntimeReplaceable {
Review comment:
Actually I think we can make this as a trait w/ switching `AggregateFunction` to a trait.
```diff
-abstract class AggregateFunction extends Expression {
+trait AggregateFunction extends Expression {
```
this at least compiles fine but I would prefer to do it separately. I will follow up after this gets merged.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809703070
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
Review comment:
Is `RuntimeReplaceable` replaced by the optimizer or the analyzer? Below it says by optimizer, but here seems by the analyzer?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809833315
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/CountIf.scala
##########
@@ -36,30 +34,11 @@ import org.apache.spark.sql.types.{AbstractDataType, BooleanType, DataType, Long
""",
group = "agg_funcs",
since = "3.0.0")
-case class CountIf(predicate: Expression) extends UnevaluableAggregate with ImplicitCastInputTypes
- with UnaryLike[Expression] {
-
- override def prettyName: String = "count_if"
-
- override def child: Expression = predicate
-
- override def nullable: Boolean = false
-
- override def dataType: DataType = LongType
-
+case class CountIf(child: Expression) extends RuntimeReplaceableAggregate
+ with ImplicitCastInputTypes with UnaryLike[Expression] {
+ override lazy val replacement: Expression = Count(new NullIf(child, Literal.FalseLiteral))
+ override def nodeName: String = "count_if"
Review comment:
ditto
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -388,29 +395,13 @@ trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
* with other databases. For example, we use this to support every, any/some aggregates by rewriting
* them with Min and Max respectively.
*/
-trait UnevaluableAggregate extends DeclarativeAggregate {
-
- override def nullable: Boolean = true
-
- override lazy val aggBufferAttributes =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "aggBufferAttributes", this)
-
- override lazy val initialValues: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "initialValues", this)
-
- override lazy val updateExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "updateExpressions", this)
-
- override lazy val mergeExpressions: Seq[Expression] =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "mergeExpressions", this)
-
- override lazy val evaluateExpression: Expression =
- throw QueryExecutionErrors.evaluateUnevaluableAggregateUnsupportedError(
- "evaluateExpression", this)
+abstract class RuntimeReplaceableAggregate extends AggregateFunction with RuntimeReplaceable {
+ def aggBufferSchema: StructType = throw new IllegalStateException(
+ "RuntimeReplaceableAggregate.aggBufferSchema should not be called")
+ def aggBufferAttributes: Seq[AttributeReference] = throw new IllegalStateException(
Review comment:
Nit: add blank lines between methods as per https://github.com/databricks/scala-style-guide#blanklines
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809154791
##########
File path: sql/core/src/test/resources/sql-tests/results/ansi/map.sql.out
##########
@@ -74,7 +74,7 @@ select map_contains_key(map('1', 'a', '2', 'b'), 1)
struct<>
-- !query output
org.apache.spark.sql.AnalysisException
-cannot resolve 'array_contains(map_keys(map('1', 'a', '2', 'b')), 1)' due to data type mismatch: Input to function array_contains should have been array followed by a value with same element type, but it's [array<string>, int].; line 1 pos 7
+cannot resolve 'map_contains_key(map('1', 'a', '2', 'b'), 1)' due to data type mismatch: Input to function map_contains_key should have been map followed by a value with same key type, but it's [map<string,string>, int].; line 1 pos 7
Review comment:
better error message
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809597967
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
Review comment:
Oh, okay because it replaces `child` to `replacement` so it triggers other related rules.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809596037
##########
File path: examples/src/main/scala/org/apache/spark/examples/extensions/AgeExample.scala
##########
@@ -18,14 +18,15 @@
package org.apache.spark.examples.extensions
import org.apache.spark.sql.catalyst.expressions.{CurrentDate, Expression, RuntimeReplaceable, SubtractDates}
+import org.apache.spark.sql.catalyst.trees.UnaryLike
/**
* How old are you in days?
*/
-case class AgeExample(birthday: Expression, child: Expression) extends RuntimeReplaceable {
-
- def this(birthday: Expression) = this(birthday, SubtractDates(CurrentDate(), birthday))
- override def exprsReplaced: Seq[Expression] = Seq(birthday)
-
- override protected def withNewChildInternal(newChild: Expression): Expression = copy(newChild)
+case class AgeExample(birthday: Expression) extends RuntimeReplaceable with UnaryLike[Expression] {
+ lazy val replacement: Expression = SubtractDates(CurrentDate(), birthday)
Review comment:
I think it's best to add `override` explicitly.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809701139
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
##########
@@ -126,8 +126,8 @@ object RaiseError {
""",
since = "2.0.0",
group = "misc_funcs")
-case class AssertTrue(left: Expression, right: Expression, child: Expression)
- extends RuntimeReplaceable {
+case class AssertTrue(left: Expression, right: Expression, replacement: Expression)
+ extends RuntimeReplaceableInheritingTypeCoercion {
Review comment:
> See the classdoc of `RuntimeReplaceableInheritingTypeCoercion`. `replacement` must be in the constructor, so that catalyst rules can transform it.
Got it, thanks
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809704476
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
+ // `RuntimeReplaceable` to define the combination. See `ParseToDate` as an example.
+ // We can also inherit the analysis behavior from the replacement expression, by
+ // mixing `InheritAnalysisRules`. See `TryAdd` as an example.
+ // - Similarly, we can use `RuntimeReplaceableAggregate` to implement new aggregate functions.
Review comment:
What is "inherit the analysis behavior"? `InheritAnalysisRules` seems just making replacement as child expression. I looked at `TryAdd` and didn't see any other than that. Does this mean the analyzer can transform down to the child expression?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809706198
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
+ // `RuntimeReplaceable` to define the combination. See `ParseToDate` as an example.
+ // We can also inherit the analysis behavior from the replacement expression, by
+ // mixing `InheritAnalysisRules`. See `TryAdd` as an example.
+ // - Similarly, we can use `RuntimeReplaceableAggregate` to implement new aggregate functions.
Review comment:
Can `RuntimeReplaceableAggregate` be mixed with `InheritAnalysisRules` too?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xinrong-databricks commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
xinrong-databricks commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809762255
##########
File path: sql/core/src/test/resources/sql-tests/inputs/string-functions.sql
##########
@@ -141,14 +141,17 @@ select to_binary('abc');
select to_binary('abc', 'utf-8');
select to_binary('abc', 'base64');
select to_binary('abc', 'hex');
+-- 'format' parameter can be any foldable string value, not just literal.
Review comment:
Thanks! That's more clear.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r810312715
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
Review comment:
I found one scenario that when reusing existing expressions, existing expression can satisfy most of the specification but has probably one different behavior on an edge cage. I am not sure what is the best way to handle it.
One could be introducing a flag to existing expression to control a specific edge case, or because of that minor diff we have to copy code but change slightly (thus still introduce new Expression but with duplicate code).
I just bring out what I saw when applying this idea. In the future I hope I can see more examples of how to deal with the scenario properly.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809638749
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala
##########
@@ -110,19 +108,19 @@ case class TryAdd(left: Expression, right: Expression, child: Expression)
since = "3.2.0",
group = "math_funcs")
// scalastyle:on line.size.limit
-case class TryDivide(left: Expression, right: Expression, child: Expression)
- extends RuntimeReplaceable {
- def this(left: Expression, right: Expression) =
+case class TryDivide(left: Expression, right: Expression, replacement: Expression)
+ extends RuntimeReplaceableInheritingTypeCoercion {
+ def this(left: Expression, right: Expression) = {
Review comment:
Can remove the `{}`
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
##########
@@ -126,8 +126,8 @@ object RaiseError {
""",
since = "2.0.0",
group = "misc_funcs")
-case class AssertTrue(left: Expression, right: Expression, child: Expression)
- extends RuntimeReplaceable {
+case class AssertTrue(left: Expression, right: Expression, replacement: Expression)
+ extends RuntimeReplaceableInheritingTypeCoercion {
Review comment:
Do we need to remain such constructor for such cases?
Why not just like
```
case class AssertTrue(left: Expression, right: Expression)
extends RuntimeReplaceableInheritingTypeCoercion {
lazy val replacement: Expression = If(left, Literal(null), RaiseError(right))
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809597467
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
Review comment:
Hm, why is it called "type coercion"? From a cursory look, it seems like it reduces the duplication for pretty SQL string.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
Review comment:
Oh, okay because it replaces `child` to `replacement` so it triggers other related rules.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809705582
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
+ // - The function can be implemented by combining some existing expressions. We can use
+ // `RuntimeReplaceable` to define the combination. See `ParseToDate` as an example.
+ // We can also inherit the analysis behavior from the replacement expression, by
+ // mixing `InheritAnalysisRules`. See `TryAdd` as an example.
+ // - Similarly, we can use `RuntimeReplaceableAggregate` to implement new aggregate functions.
Review comment:
> just making replacement as child expression
Yea that's the key. It's a child so can be transformed by analysis rules, and we can inherit any analysis behavior from te replacement expression, which is mostly type coercion behavior.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809704905
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##########
@@ -324,7 +326,36 @@ object FunctionRegistry {
val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")
- // Note: Whenever we add a new entry here, make sure we also update ExpressionToSQLSuite
+ // ==============================================================================================
+ // The guideline for adding SQL functions
+ // ==============================================================================================
+ // To add a SQL function, we usually need to create a new `Expression` for the function, and
+ // implement the function logic in both the interpretation code path and codegen code path of the
+ // `Expression`. We also need to define the type coercion behavior for the function inputs, by
+ // extending `ImplicitCastInputTypes` or updating type coercion rules directly.
+ //
+ // It's much simpler if the SQL function can be implemented with existing expression(s). There are
+ // a few cases:
+ // - The function is simply an alias of another function. We can just register the same
+ // expression with a different function name, e.g. `expression[Rand]("random", true)`.
+ // - The function is mostly the same with another function, but has a different parameter list.
+ // We can use `RuntimeReplaceable` to create a new expression, which can customize the
+ // parameter list and analysis behavior (type coercion). The `RuntimeReplaceable` expression
+ // will be replaced by the actual expression at the end of analysis. See `Left` as an example.
Review comment:
It's replaced by the first batch in optimizer: "finish analysis", so it's kind of "end of analysis"
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809629257
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
Review comment:
I'm not sure. It looks like we also could override `inputTypes` in `RuntimeReplaceable`.
So, do we really need this trait ?
##########
File path: sql/core/src/test/resources/sql-functions/sql-expression-schema.md
##########
@@ -182,8 +181,8 @@
| org.apache.spark.sql.catalyst.expressions.MakeDate | make_date | SELECT make_date(2013, 7, 15) | struct<make_date(2013, 7, 15):date> |
| org.apache.spark.sql.catalyst.expressions.MakeInterval | make_interval | SELECT make_interval(100, 11, 1, 1, 12, 30, 01.001001) | struct<make_interval(100, 11, 1, 1, 12, 30, 1.001001):interval> |
| org.apache.spark.sql.catalyst.expressions.MakeTimestamp | make_timestamp | SELECT make_timestamp(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp(2014, 12, 28, 6, 30, 45.887):timestamp> |
-| org.apache.spark.sql.catalyst.expressions.MakeTimestampLTZ | make_timestamp_ltz | SELECT make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887):timestamp> |
-| org.apache.spark.sql.catalyst.expressions.MakeTimestampNTZ | make_timestamp_ntz | SELECT make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887):timestamp_ntz> |
+| org.apache.spark.sql.catalyst.expressions.MakeTimestampLTZExpressionBuilder$ | make_timestamp_ltz | SELECT make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ltz(2014, 12, 28, 6, 30, 45.887):timestamp> |
+| org.apache.spark.sql.catalyst.expressions.MakeTimestampNTZExpressionBuilder$ | make_timestamp_ntz | SELECT make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887) | struct<make_timestamp_ntz(2014, 12, 28, 6, 30, 45.887):timestamp_ntz> |
Review comment:
Although MakeTimestampNTZExpressionBuilder looks useful, but seems strange here.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
##########
@@ -405,30 +404,32 @@ case class AesDecrypt(
input: Expression,
key: Expression,
mode: Expression,
- padding: Expression,
- child: Expression)
- extends RuntimeReplaceable {
-
- def this(input: Expression, key: Expression, mode: Expression, padding: Expression) = {
- this(
- input,
- key,
- mode,
- padding,
- StaticInvoke(
- classOf[ExpressionImplUtils],
- BinaryType,
- "aesDecrypt",
- Seq(input, key, mode, padding),
- Seq(BinaryType, BinaryType, StringType, StringType)))
- }
+ padding: Expression)
+ extends RuntimeReplaceable with ImplicitCastInputTypes {
+
+ lazy val replacement: Expression = StaticInvoke(
+ classOf[ExpressionImplUtils],
+ BinaryType,
+ "aesDecrypt",
+ Seq(input, key, mode, padding),
+ inputTypes)
+
def this(input: Expression, key: Expression, mode: Expression) =
this(input, key, mode, Literal("DEFAULT"))
def this(input: Expression, key: Expression) =
this(input, key, Literal("GCM"))
- def exprsReplaced: Seq[Expression] = Seq(input, key)
Review comment:
Yes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809643242
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
+ extends RuntimeReplaceable with UnaryLike[Expression] {
+ override def child: Expression = replacement
+ def actualInputs: Seq[Expression]
Review comment:
How about `actualInputs` -> `arguments`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35534: [SPARK-38240][SQL] Improve RuntimeReplaceable and add a guideline for adding new functions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35534:
URL: https://github.com/apache/spark/pull/35534#discussion_r809644899
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -352,34 +352,44 @@ trait Unevaluable extends Expression {
* An expression that gets replaced at runtime (currently by the optimizer) into a different
* expression for evaluation. This is mainly used to provide compatibility with other databases.
* For example, we use this to support "nvl" by replacing it with "coalesce".
- *
- * A RuntimeReplaceable should have the original parameters along with a "child" expression in the
- * case class constructor, and define a normal constructor that accepts only the original
- * parameters. For an example, see [[Nvl]]. To make sure the explain plan and expression SQL
- * works correctly, the implementation should also override flatArguments method and sql method.
*/
-trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
- override def nullable: Boolean = child.nullable
- override def dataType: DataType = child.dataType
+trait RuntimeReplaceable extends Expression {
+ def replacement: Expression
+
+ override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ override def nullable: Boolean = replacement.nullable
+ override def dataType: DataType = replacement.dataType
// As this expression gets replaced at optimization with its `child" expression,
// two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
// are semantically equal.
- override lazy val preCanonicalized: Expression = child.preCanonicalized
-
- /**
- * Only used to generate SQL representation of this expression.
- *
- * Implementations should override this with original parameters
- */
- def exprsReplaced: Seq[Expression]
-
- override def sql: String = mkString(exprsReplaced.map(_.sql))
+ override lazy val preCanonicalized: Expression = replacement.preCanonicalized
- final override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
+ final override def eval(input: InternalRow = null): Any =
+ throw QueryExecutionErrors.cannotEvaluateExpressionError(this)
+ final override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode =
+ throw QueryExecutionErrors.cannotGenerateCodeForExpressionError(this)
+}
- def mkString(childrenString: Seq[String]): String = {
- prettyName + childrenString.mkString("(", ", ", ")")
+/**
+ * A special variant of [[RuntimeReplaceable]]. It makes `replacement` the child of the expression,
+ * to inherit the type coercion rules for it. The implementation should put `replacement` in the
+ * case class constructor, and define a normal constructor that accepts only the original
+ * parameters, or use `ExpressionBuilder`. For an example, see [[TryAdd]]. To make sure the explain
+ * plan and expression SQL works correctly, the implementation should also implement the
+ * `actualInputs` method, which should extracts the actual inputs after type coercion from
+ * `replacement`.
+ */
+trait RuntimeReplaceableInheritingTypeCoercion
+ extends RuntimeReplaceable with UnaryLike[Expression] {
Review comment:
And `RuntimeReplaceableAggregate` can leverage this too in the future.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org