-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-23736][SQL] Extending the concat function to support array columns #20858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
282e724
aa5a089
90d3ab7
bb46c3d
11205af
753499d
2efdd77
fd84bee
116f91f
e199ac5
067c2db
090929f
8abd1a8
367ee22
6bb33e6
57b250c
944e0c9
7f5124b
0201e4b
600ae89
f2a67e8
8a125d9
5a4cc8c
f7bdcf7
36d5d25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -408,6 +408,7 @@ object FunctionRegistry { | |
| expression[MapValues]("map_values"), | ||
| expression[Size]("size"), | ||
| expression[SortArray]("sort_array"), | ||
| expression[ConcatArrays]("concat_arrays"), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not reusing concat(array1, array2, ..., arrayN) -> array ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've already played with this option in my mind, but I'm not sure how concat would be categorized. Currently, concat is defined as a pure string operation: Whereas the functionality in this PR belongs rather to the collection_funcs group. Having just one function for both expressions would be elegant, but can you advise what group should be assigned to concat?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about move it to collection functions?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, will merge the functions into one. Do you find having one expression class concatenation per the concatenation type ok? I'm afraid if I incorporate all the logic into one expression class then the code will become messy since each codeGen and eveluation has a different nature. |
||
| CreateStruct.registryEntry, | ||
|
|
||
| // misc functions | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -699,3 +699,88 @@ abstract class TernaryExpression extends Expression { | |
| * and Hive function wrappers. | ||
| */ | ||
| trait UserDefinedExpression | ||
|
|
||
| /** | ||
| * The trait covers logic for performing null save evaluation and code generation. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: null safe. |
||
| */ | ||
| trait NullSafeEvaluation extends Expression | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to bring in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
| { | ||
| override def foldable: Boolean = children.forall(_.foldable) | ||
|
|
||
| override def nullable: Boolean = children.exists(_.nullable) | ||
|
|
||
| /** | ||
| * Default behavior of evaluation according to the default nullability of NullSafeEvaluation. | ||
| * If a class utilizing NullSaveEvaluation override [[nullable]], probably should also | ||
| * override this. | ||
| */ | ||
| override def eval(input: InternalRow): Any = | ||
| { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spark usually use the style like: override def eval(input: InternalRow): Any = {
val values = children.map(_.eval(input))
if (values.contains(null)) {
null
} else {
nullSafeEval(values)
}
}You could follow the style of other codes.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are other places where the braces
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think I fixed all style differences.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems the style fix is missed here. |
||
| val values = children.map(_.eval(input)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably don't need to evaluate all children. Once any child expression is null, we can just return null. |
||
| if (values.contains(null)) null | ||
| else nullSafeEval(values) | ||
| } | ||
|
|
||
| /** | ||
| * Called by default [[eval]] implementation. If a class utilizing NullSaveEvaluation keep | ||
| * the default nullability, they can override this method to save null-check code. If we need | ||
| * full control of evaluation process, we should override [[eval]]. | ||
| */ | ||
| protected def nullSafeEval(inputs: Seq[Any]): Any = | ||
| sys.error(s"The class utilizing NullSaveEvaluation must override either eval or nullSafeEval") | ||
|
|
||
| /** | ||
| * Short hand for generating of null save evaluation code. | ||
| * If either of the sub-expressions is null, the result of this computation | ||
| * is assumed to be null. | ||
| * | ||
| * @param f accepts a sequence of variable names and returns Java code to compute the output. | ||
| */ | ||
| protected def defineCodeGen( | ||
| ctx: CodegenContext, | ||
| ev: ExprCode, | ||
| f: Seq[String] => String): ExprCode = { | ||
| nullSafeCodeGen(ctx, ev, values => { | ||
| s"${ev.value} = ${f(values)};" | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Called by expressions to generate null safe evaluation code. | ||
| * If either of the sub-expressions is null, the result of this computation | ||
| * is assumed to be null. | ||
| * | ||
| * @param f a function that accepts a sequence of non-null evaluation result names of children | ||
| * and returns Java code to compute the output. | ||
| */ | ||
| protected def nullSafeCodeGen( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method looks almost the same with the one in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will combine it with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WeichenXu123 I do agree that there are strong similarities in the code. If you take a look at
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel it's ok to discuss this in follow-up activities cuz this is less related to this pr. So, can you make this pr minimal as much as possible?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, will try. |
||
| ctx: CodegenContext, | ||
| ev: ExprCode, | ||
| f: Seq[String] => String): ExprCode = { | ||
| val gens = children.map(_.genCode(ctx)) | ||
| val resultCode = f(gens.map(_.value)) | ||
|
|
||
| if (nullable) { | ||
| val nullSafeEval = | ||
| (s""" | ||
| ${ev.isNull} = false; // resultCode could change nullability. | ||
| $resultCode | ||
| """ /: children.zip(gens)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| case (acc, (child, gen)) => | ||
| gen.code + ctx.nullSafeExec(child.nullable, gen.isNull)(acc) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example, for a binary expression, doesn't this generate code like: rightGen.code + ctx.nullSafeExec(right.nullable, rightGen.isNull) {
leftGen.code + ctx.nullSafeExec(left.nullable, leftGen.isNull) {
${ev.isNull} = false; // resultCode could change nullability.
$resultCode
}
}Although for deterministic expressions, the evaluation order doesn't matter. But for non-deterministic, I'm little concerned that it may cause unexpected change. |
||
| } | ||
|
|
||
| ev.copy(code = s""" | ||
| boolean ${ev.isNull} = true; | ||
| ${CodeGenerator.javaType(dataType)} ${ev.value} = ${CodeGenerator.defaultValue(dataType)}; | ||
| $nullSafeEval | ||
| """) | ||
| } else { | ||
| ev.copy(code = s""" | ||
| boolean ${ev.isNull} = false; | ||
| ${gens.map(_.code).mkString("\n")} | ||
| ${CodeGenerator.javaType(dataType)} ${ev.value} = ${CodeGenerator.defaultValue(dataType)}; | ||
| $resultCode""", isNull = "false") | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -21,8 +21,10 @@ import java.util.Comparator | |||
| import org.apache.spark.sql.catalyst.InternalRow | ||||
| import org.apache.spark.sql.catalyst.analysis.TypeCheckResult | ||||
| import org.apache.spark.sql.catalyst.expressions.codegen.{CodegenContext, CodeGenerator, CodegenFallback, ExprCode} | ||||
| import org.apache.spark.sql.catalyst.util.{ArrayData, GenericArrayData, MapData} | ||||
| import org.apache.spark.sql.catalyst.util.{ArrayData, GenericArrayData, MapData, TypeUtils} | ||||
| import org.apache.spark.sql.types._ | ||||
| import org.apache.spark.unsafe.Platform | ||||
| import org.apache.spark.unsafe.array.ByteArrayMethods | ||||
|
|
||||
| /** | ||||
| * Given an array or map, returns its size. Returns -1 if null. | ||||
|
|
@@ -287,3 +289,152 @@ case class ArrayContains(left: Expression, right: Expression) | |||
|
|
||||
| override def prettyName: String = "array_contains" | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Concatenates multiple arrays into one. | ||||
| */ | ||||
| @ExpressionDescription( | ||||
| usage = "_FUNC_(expr, ...) - Concatenates multiple arrays into one.", | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defines that the element types of the arrays must be the same. |
||||
| examples = """ | ||||
| Examples: | ||||
| > SELECT _FUNC_(array(1, 2, 3), array(4, 5), array(6)); | ||||
| [1,2,3,4,5,6] | ||||
| """) | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we add |
||||
| case class ConcatArrays(children: Seq[Expression]) extends Expression with NullSafeEvaluation { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a common base class (e.g., spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala Line 649 in e4bec7c
|
||||
|
|
||||
| override def checkInputDataTypes(): TypeCheckResult = { | ||||
| val arrayCheck = checkInputDataTypesAreArrays | ||||
| if(arrayCheck.isFailure) arrayCheck | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style issue: if (...) {
...
} else {
...
} |
||||
| else TypeUtils.checkForSameTypeInputExpr(children.map(_.dataType), s"function $prettyName") | ||||
| } | ||||
|
|
||||
| private def checkInputDataTypesAreArrays(): TypeCheckResult = | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just put this in |
||||
| { | ||||
| val mismatches = children.zipWithIndex.collect { | ||||
| case (child, idx) if !ArrayType.acceptsType(child.dataType) => | ||||
| s"argument ${idx + 1} has to be ${ArrayType.simpleString} type, " + | ||||
| s"however, '${child.sql}' is of ${child.dataType.simpleString} type." | ||||
| } | ||||
|
|
||||
| if (mismatches.isEmpty) { | ||||
| TypeCheckResult.TypeCheckSuccess | ||||
| } else { | ||||
| TypeCheckResult.TypeCheckFailure(mismatches.mkString(" ")) | ||||
| } | ||||
| } | ||||
|
|
||||
| override def dataType: ArrayType = | ||||
| children | ||||
| .headOption.map(_.dataType.asInstanceOf[ArrayType]) | ||||
| .getOrElse(ArrayType.defaultConcreteType.asInstanceOf[ArrayType]) | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we allow empty children? I can't think of a use case for now and we should better disallow it first.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely share your opinion, but I think we should be consistent across the whole Spark SQL API. Functions like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm .. but then this is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, changing to return type |
||||
|
|
||||
|
|
||||
| override protected def nullSafeEval(inputs: Seq[Any]): Any = { | ||||
| val elements = inputs.flatMap(_.asInstanceOf[ArrayData].toObjectArray(dataType.elementType)) | ||||
| new GenericArrayData(elements) | ||||
| } | ||||
|
|
||||
| override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = { | ||||
| nullSafeCodeGen(ctx, ev, arrays => { | ||||
| val elementType = dataType.elementType | ||||
| if (CodeGenerator.isPrimitiveType(elementType)) { | ||||
| genCodeForConcatOfPrimitiveElements(ctx, elementType, arrays, ev.value) | ||||
| } else { | ||||
| genCodeForConcatOfComplexElements(ctx, arrays, ev.value) | ||||
| } | ||||
| }) | ||||
| } | ||||
|
|
||||
| private def genCodeForNumberOfElements( | ||||
| ctx: CodegenContext, | ||||
| elements: Seq[String] | ||||
| ) : (String, String) = { | ||||
| val variableName = ctx.freshName("numElements") | ||||
| val code = elements | ||||
| .map(el => s"$variableName += $el.numElements();") | ||||
| .foldLeft( s"int $variableName = 0;")((acc, s) => acc + "\n" + s) | ||||
| (code, variableName) | ||||
| } | ||||
|
|
||||
| private def genCodeForConcatOfPrimitiveElements( | ||||
| ctx: CodegenContext, | ||||
| elementType: DataType, | ||||
| elements: Seq[String], | ||||
| arrayDataName: String | ||||
| ): String = { | ||||
| val arrayName = ctx.freshName("array") | ||||
| val arraySizeName = ctx.freshName("size") | ||||
| val counter = ctx.freshName("counter") | ||||
| val tempArrayDataName = ctx.freshName("tempArrayData") | ||||
|
|
||||
| val (numElemCode, numElemName) = genCodeForNumberOfElements(ctx, elements) | ||||
|
|
||||
| val unsafeArraySizeInBytes = s""" | ||||
| |int $arraySizeName = UnsafeArrayData.calculateHeaderPortionInBytes($numElemName) + | ||||
| |${classOf[ByteArrayMethods].getName}.roundNumberOfBytesToNearestWord( | ||||
| |${elementType.defaultSize} * $numElemName | ||||
| |); | ||||
| """.stripMargin | ||||
| val baseOffset = Platform.BYTE_ARRAY_OFFSET | ||||
|
|
||||
| val primitiveValueTypeName = CodeGenerator.primitiveTypeName(elementType) | ||||
| val assignments = elements.map { el => | ||||
| s""" | ||||
| |for(int z = 0; z < $el.numElements(); z++) { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stype: |
||||
| | if($el.isNullAt(z)) { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: |
||||
| | $tempArrayDataName.setNullAt($counter); | ||||
| | } else { | ||||
| | $tempArrayDataName.set$primitiveValueTypeName( | ||||
| | $counter, | ||||
| | $el.get$primitiveValueTypeName(z) | ||||
| | ); | ||||
| | } | ||||
| | $counter++; | ||||
| |} | ||||
| """.stripMargin | ||||
| }.mkString("\n") | ||||
|
|
||||
| s""" | ||||
| |$numElemCode | ||||
| |$unsafeArraySizeInBytes | ||||
| |byte[] $arrayName = new byte[$arraySizeName]; | ||||
| |UnsafeArrayData $tempArrayDataName = new UnsafeArrayData(); | ||||
| |Platform.putLong($arrayName, $baseOffset, $numElemName); | ||||
| |$tempArrayDataName.pointTo($arrayName, $baseOffset, $arraySizeName); | ||||
| |int $counter = 0; | ||||
| |$assignments | ||||
| |$arrayDataName = $tempArrayDataName; | ||||
| """.stripMargin | ||||
|
|
||||
| } | ||||
|
|
||||
| private def genCodeForConcatOfComplexElements( | ||||
| ctx: CodegenContext, | ||||
| elements: Seq[String], | ||||
| arrayDataName: String | ||||
| ): String = { | ||||
| val genericArrayClass = classOf[GenericArrayData].getName | ||||
| val arrayName = ctx.freshName("arrayObject") | ||||
| val counter = ctx.freshName("counter") | ||||
| val (numElemCode, numElemName) = genCodeForNumberOfElements(ctx, elements) | ||||
|
|
||||
| val assignments = elements.map { el => | ||||
| s""" | ||||
| |for(int z = 0; z < $el.numElements(); z++) { | ||||
| | $arrayName[$counter] = $el.array()[z]; | ||||
| | $counter++; | ||||
| |} | ||||
| """.stripMargin | ||||
| }.mkString("\n") | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To use |
||||
|
|
||||
| s""" | ||||
| |$numElemCode | ||||
| |Object[] $arrayName = new Object[$numElemName]; | ||||
| |int $counter = 0; | ||||
| |$assignments | ||||
| |$arrayDataName = new $genericArrayClass($arrayName); | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we concate complex elements into UnsafeArrayData?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, can we reuse the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really like this idea! I think it would require moving the complex type insertion logic from Also see that we could improve codeGen of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You couldn't use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, currently there are no |
||||
| """.stripMargin | ||||
| } | ||||
|
|
||||
| override def prettyName: String = "concat_arrays" | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3046,6 +3046,14 @@ object functions { | |
| ArrayContains(column.expr, Literal(value)) | ||
| } | ||
|
|
||
| /** | ||
| * Merges multiple arrays into one by putting elements from the specific array after elements | ||
| * from the previous array. If any of the arrays is null, null is returned. | ||
| * @group collection_funcs | ||
| * @since 2.4.0 | ||
| */ | ||
| def concat_arrays(columns: Column*): Column = withExpr { ConcatArrays(columns.map(_.expr)) } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to add this func. in |
||
|
|
||
| /** | ||
| * Creates a new row for each element in the given array or map column. | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we note
colsare expected to be array type?