You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhengruifeng (via GitHub)" <gi...@apache.org> on 2023/02/14 03:13:02 UTC
[GitHub] [spark] zhengruifeng commented on a diff in pull request #38947: [SPARK-41233][SQL] Add `array_prepend` function
zhengruifeng commented on code in PR #38947:
URL: https://github.com/apache/spark/pull/38947#discussion_r1105245346
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -1399,6 +1399,149 @@ case class ArrayContains(left: Expression, right: Expression)
copy(left = newLeft, right = newRight)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage =
+ "_FUNC_(array, value) - Returns an array containing value as well as all elements from array. The new element is positioned at the beginning of the array.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(array(1, 2, 3), 4);
+ [4, 1, 2, 3]
+ """,
+ group = "array_funcs",
+ since = "3.4.0")
+case class ArrayPrepend(left: Expression, right: Expression)
+ extends BinaryExpression
+ with ImplicitCastInputTypes
+ with ComplexTypeMergingExpression
+ with QueryErrorsBase {
+
+ override def nullable: Boolean = left.nullable
+
+ override def eval(input: InternalRow): Any = {
+ val value1 = left.eval(input)
+ if (value1 == null) {
+ null
+ } else {
+ val value2 = right.eval(input)
+ nullSafeEval(value1, value2)
+ }
+ }
+ override def nullSafeEval(arr: Any, value: Any): Any = {
+ val numberOfElements = arr.asInstanceOf[ArrayData].numElements()
+ if (numberOfElements + 1 > ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH) {
+ throw QueryExecutionErrors.concatArraysWithElementsExceedLimitError(numberOfElements)
+ }
+ val newArray = new Array[Any](numberOfElements + 1)
+ newArray(0) = value
+ var pos = 1
+ arr
+ .asInstanceOf[ArrayData]
+ .foreach(
+ right.dataType,
+ (i, v) => {
+ newArray(pos) = v
+ pos += 1
+ })
+ new GenericArrayData(newArray)
+ }
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ val leftGen = left.genCode(ctx)
+ val rightGen = right.genCode(ctx)
+ val f = (arr: String, value: String) => {
+ val newArraySize = ctx.freshName("newArraySize")
+ val newArray = ctx.freshName("newArray")
+ val i = ctx.freshName("i")
+ val pos = ctx.freshName("pos")
+ val allocation = CodeGenerator.createArrayData(
+ newArray,
+ right.dataType,
+ newArraySize,
+ s" $prettyName failed.")
+ val assignment =
+ CodeGenerator.createArrayAssignment(newArray, right.dataType, arr, pos, i, false)
+ val newElemAssignment =
+ CodeGenerator.setArrayElement(newArray, right.dataType, pos, value, Some(rightGen.isNull))
Review Comment:
I think we should not use `right.dataType` in `assignment ` and `newElemAssignment `.
may use the
```
@transient protected lazy val elementType: DataType =
inputTypes.head.asInstanceOf[ArrayType].elementType
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -1399,6 +1399,149 @@ case class ArrayContains(left: Expression, right: Expression)
copy(left = newLeft, right = newRight)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage =
+ "_FUNC_(array, value) - Returns an array containing value as well as all elements from array. The new element is positioned at the beginning of the array.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(array(1, 2, 3), 4);
+ [4, 1, 2, 3]
+ """,
+ group = "array_funcs",
+ since = "3.4.0")
+case class ArrayPrepend(left: Expression, right: Expression)
+ extends BinaryExpression
+ with ImplicitCastInputTypes
+ with ComplexTypeMergingExpression
+ with QueryErrorsBase {
+
+ override def nullable: Boolean = left.nullable
+
+ override def eval(input: InternalRow): Any = {
+ val value1 = left.eval(input)
+ if (value1 == null) {
+ null
+ } else {
+ val value2 = right.eval(input)
+ nullSafeEval(value1, value2)
+ }
+ }
+ override def nullSafeEval(arr: Any, value: Any): Any = {
+ val numberOfElements = arr.asInstanceOf[ArrayData].numElements()
+ if (numberOfElements + 1 > ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH) {
+ throw QueryExecutionErrors.concatArraysWithElementsExceedLimitError(numberOfElements)
+ }
+ val newArray = new Array[Any](numberOfElements + 1)
+ newArray(0) = value
+ var pos = 1
+ arr
+ .asInstanceOf[ArrayData]
+ .foreach(
+ right.dataType,
+ (i, v) => {
+ newArray(pos) = v
+ pos += 1
+ })
+ new GenericArrayData(newArray)
+ }
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ val leftGen = left.genCode(ctx)
+ val rightGen = right.genCode(ctx)
+ val f = (arr: String, value: String) => {
+ val newArraySize = ctx.freshName("newArraySize")
+ val newArray = ctx.freshName("newArray")
+ val i = ctx.freshName("i")
+ val pos = ctx.freshName("pos")
+ val allocation = CodeGenerator.createArrayData(
+ newArray,
+ right.dataType,
+ newArraySize,
+ s" $prettyName failed.")
+ val assignment =
+ CodeGenerator.createArrayAssignment(newArray, right.dataType, arr, pos, i, false)
+ val newElemAssignment =
+ CodeGenerator.setArrayElement(newArray, right.dataType, pos, value, Some(rightGen.isNull))
+ s"""
+ |int $pos = 0;
+ |int $newArraySize = $arr.numElements() + 1;
+ |$allocation
+ |$newElemAssignment
+ |$pos = $pos + 1;
+ |for (int $i = 0; $i < $arr.numElements(); $i ++) {
+ | $assignment
+ | $pos = $pos + 1;
+ |}
+ |${ev.value} = $newArray;
+ |""".stripMargin
+ }
+ val resultCode = f(leftGen.value, rightGen.value)
+ if(nullable) {
+ val nullSafeEval = leftGen.code + rightGen.code + ctx.nullSafeExec(nullable, leftGen.isNull) {
+ s"""
+ |${ev.isNull} = false;
+ |${resultCode}
+ |""".stripMargin
+ }
+ ev.copy(code =
+ code"""
+ boolean ${ev.isNull} = true;
+ ${CodeGenerator.javaType(dataType)} ${ev.value} = ${CodeGenerator.defaultValue(dataType)};
+ $nullSafeEval
+ """)
+ } else {
+ ev.copy(code =
+ code"""
+ ${leftGen.code}
+ ${rightGen.code}
+ ${CodeGenerator.javaType(dataType)} ${ev.value} = ${CodeGenerator.defaultValue(dataType)};
+ $resultCode""", isNull = FalseLiteral)
+ }
+ }
+
+ override def prettyName: String = "array_prepend"
+
+ override protected def withNewChildrenInternal(
+ newLeft: Expression, newRight: Expression): ArrayPrepend =
+ copy(left = newLeft, right = newRight)
+
+ override def dataType: DataType = left.dataType
Review Comment:
```suggestion
override def dataType: DataType = if (right.nullable) left.dataType.asNullable else left.dataType
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -1399,6 +1399,119 @@ case class ArrayContains(left: Expression, right: Expression)
copy(left = newLeft, right = newRight)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage =
+ "_FUNC_(array, value) - Returns an array containing value as well as all elements from array. The new element is positioned at the beginning of the array.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(array(1, 2, 3), 4);
+ [4, 1, 2, 3]
+ """,
+ group = "array_funcs",
+ since = "3.4.0")
+case class ArrayPrepend(left: Expression, right: Expression)
+ extends BinaryExpression
+ with ImplicitCastInputTypes
+ with NullIntolerant
+ with QueryErrorsBase {
+
+ override def nullSafeEval(arr: Any, value: Any): Any = {
+ val numberOfElements = arr.asInstanceOf[ArrayData].numElements()
+ if (numberOfElements + 1 > ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH) {
+ throw QueryExecutionErrors.concatArraysWithElementsExceedLimitError(numberOfElements)
+ }
+ val newArray = new Array[Any](numberOfElements + 1)
+ newArray(0) = value
+ var pos = 1
+ arr
Review Comment:
would you minding make this `nullSafeEval` method and `doGenCode` method more consistent with `ArrayAppend`:
```
@transient protected lazy val elementType: DataType =
inputTypes.head.asInstanceOf[ArrayType].elementType
override protected def nullSafeEval(arr: Any, elementData: Any): Any = {
val arrayData = arr.asInstanceOf[ArrayData]
val numberOfElements = arrayData.numElements() + 1
if (numberOfElements > ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH) {
throw QueryExecutionErrors.concatArraysWithElementsExceedLimitError(numberOfElements)
}
val finalData = new Array[Any](numberOfElements)
finalData.update(0, elementData)
arrayData.foreach(elementType, (i: Int, v: Any) => finalData.update(i + 1, v))
new GenericArrayData(finalData)
}
```
I think we can make a better abstract later
--
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