sunchao commented on code in PR #56575:
URL: https://github.com/apache/spark/pull/56575#discussion_r3714070654
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2621,29 +2621,34 @@ case class Substring(str: Expression, pos: Expression,
len: Expression)
since = "2.3.0",
group = "string_funcs")
// scalastyle:on line.size.limit
-case class Right(str: Expression, len: Expression) extends RuntimeReplaceable
- with ImplicitCastInputTypes with BinaryLike[Expression] {
-
- override lazy val replacement: Expression = If(
- IsNull(str),
- Literal(null, str.dataType),
- If(
- LessThanOrEqual(len, Literal(0)),
- Literal(UTF8String.EMPTY_UTF8, str.dataType),
- new Substring(str, UnaryMinus(len, failOnError = false))
- )
- )
+object Right extends DelegateFunction {
+ override val name: String = "right"
override def inputTypes: Seq[AbstractDataType] =
- Seq(
- StringTypeWithCollation(supportsTrimCollation = true),
- IntegerType
- )
- override def left: Expression = str
- override def right: Expression = len
- override protected def withNewChildrenInternal(
- newLeft: Expression, newRight: Expression): Expression = {
- copy(str = newLeft, len = newRight)
+ Seq(StringTypeWithCollation(supportsTrimCollation = true), IntegerType)
+
+ // At build time `str` is the not-yet-coerced argument (wrapped in an
`ImplicitCastInput` marker
+ // that delegates `dataType` to its child), so `str.dataType` is the *input*
type, which is not
+ // necessarily a string yet -- e.g. `right(12345, 2)` has an `IntegerType`
child the implicit cast
+ // will turn into a string. Use it for the null/empty branch literals only
when it is already a
+ // string-family type, so a CHAR(N)/VARCHAR(N) result (under
+ // `spark.sql.preserveCharVarcharTypeInfo`) or a non-default collation is
preserved through the
+ // `If` branch unification; otherwise fall back to plain `StringType`, the
type the implicit cast
+ // produces. Typing a UTF8String literal with a non-string type would be
invalid.
+ override def lower(args: Seq[Expression]): Expression = {
+ val str = args(0)
+ val len = args(1)
+ val litType = str.dataType match {
+ case _: StringType | _: CharType | _: VarcharType => str.dataType
+ case _ => StringType
+ }
+ If(
+ IsNull(str),
+ Literal(null, litType),
+ If(
+ LessThanOrEqual(len, Literal(0)),
+ Literal(UTF8String.EMPTY_UTF8, litType),
+ new Substring(str, UnaryMinus(len, failOnError = false))))
Review Comment:
[P2] Keep window inputs single-use before extraction
Because this definition is constructed during function resolution, `str` is
present under both `IsNull(str)` and `Substring(str, ...)` before
`ExtractWindowExpressions` runs. On the base branch, `Right` exposes the input
only once during analysis and expands its replacement later in
`FinishAnalysis.ReplaceExpressions`.
```sql
SELECT right(
listagg(CAST(id AS STRING), chr(44))
OVER (ORDER BY id ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW),
1
)
FROM range(5)
```
This extracts two distinct window outputs (`_we0` and `_we1`) instead of
one. Both remain referenced, and each gets a separate aggregate buffer,
doubling computation and potentially unbounded `listagg` memory. Could we
preserve a single analyzer-visible window input or deduplicate it before
extraction, with a physical-plan regression asserting that only one window
aggregate is generated?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
