[SPARK-41233][FOLLOWUP] Refactor `array_prepend` with `RuntimeReplaceable` #40563

beliefer · 2023-03-27T10:23:53Z

What changes were proposed in this pull request?

Recently, Spark SQL supported array_insert and array_prepend. All implementations are individual.
In fact, array_prepend is special case of array_insert and we can reuse the array_insert by extends RuntimeReplaceable.

Why are the changes needed?

Simplify the implementation of array_prepend.

Does this PR introduce any user-facing change?

'No'.
Just update the inner implementation.

How was this patch tested?

Exists test case.

LuciferYang · 2023-03-27T14:38:06Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-    with ImplicitCastInputTypes
-    with ComplexTypeMergingExpression
-    with QueryErrorsBase {
+trait ArrayInsertEnd extends RuntimeReplaceable


should we rename this trait? ArrayInsertEnd cannot describe ArrayPrepend well

LuciferYang · 2023-03-28T03:53:38Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-      nullSafeEval(value1, value2)
-    }
+  override protected def withNewChildrenInternal(
+      newLeft: Expression, newRight: Expression): Expression = {


return type change to ArrayAppend?

LuciferYang · 2023-03-28T03:53:59Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala


+  override protected def withNewChildrenInternal(
+      newLeft: Expression, newRight: Expression): Expression = {


return type change to ArrayPrepend?

LuciferYang · 2023-03-28T07:02:18Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-  @transient protected lazy val elementType: DataType =
-    inputTypes.head.asInstanceOf[ArrayType].elementType
+  override lazy val replacement: Expression =
+    ArrayInsert(arr, Add(ArraySize(left), Literal(1)), ele)


use both arr and left?

LuciferYang · 2023-03-28T14:01:38Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

@@ -1855,50 +1855,6 @@ class CollectionExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper
    checkEvaluation(ArrayRepeat(Literal("hi"), Literal(null, IntegerType)), null)
  }

-  test("SPARK-41233: ArrayPrepend") {


hmm... please ignore the comments above

Any reason to delete the test cases? Should be still relevant right?

Any reason to delete the test cases? Should be still relevant right?

Runtimereplace do not support execute and it will be replaced with other expression.

I hope we have reviewed that we are having all types of test cases deleted here in DataFrameFunctionsSuite. If we are missing, please help add those.

I checked and added test cases in DataFrameFunctionsSuite

beliefer · 2023-03-29T03:14:18Z

ping @zhengruifeng @HyukjinKwon @dongjoon-hyun cc @infoankitp @navinvishy

navinvishy · 2023-03-29T03:57:40Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

@@ -1855,50 +1855,6 @@ class CollectionExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper
    checkEvaluation(ArrayRepeat(Literal("hi"), Literal(null, IntegerType)), null)
  }

-  test("SPARK-41233: ArrayPrepend") {


Any reason to delete the test cases? Should be still relevant right?

navinvishy · 2023-03-29T04:02:10Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-    with QueryErrorsBase {
-
-  override def nullable: Boolean = left.nullable
+trait InsertArrayOneSide extends RuntimeReplaceable


nit: Should we just call this RuntimeReplaceableBinary or something? Just a thought, since there doesn't seem to be anything specific about arrays in this trait.

InsertArrayOneSide should be better name. Thank you.

HyukjinKwon · 2023-03-29T05:21:22Z

.../connect/common/src/test/resources/query-tests/explain-results/function_array_append.explain

@@ -1,2 +1,2 @@
-Project [array_append(e#0, 1) AS array_append(e, 1)#0]
+Project [array_insert(e#0, (size(e#0, false) + 1), 1) AS array_append(e, 1)#0]


You can fix array_insert to have getTagValue(FunctionRegistry.FUNC_ALIAS).getOrElse(name), and set the alias in FunctionRegistry by setting setAlias to true

@HyukjinKwon I tested and found this can't given help.

There are a lot golden files are the same as the case.
Refer: https://github.com/apache/spark/blob/f6d6ec138fe104bc7317993fe90bfdc672c11938/connector/connect/common/src/test/resources/query-tests/explain-results/function_map_contains_key.explain

infoankitp · 2023-03-29T13:33:33Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-
-  override def nullable: Boolean = left.nullable
+trait InsertArrayOneSide extends RuntimeReplaceable
+  with ImplicitCastInputTypes with BinaryLike[Expression] with QueryErrorsBase {


Any diff between BinaryLike[Expression] and BinaryExpression? Also, any specific reason for removing ComplexTypeMergingExpression, I think this can help in assigning values of containsNull and nullable fields ?

BinaryExpression could be evaluated. In fact, the implementation of RuntimeReplaceable could not be evaluated.
ArrayInsert already implemented the ComplexTypeMergingExpression.

Makes sense ! Thanks so much!

infoankitp · 2023-03-29T13:42:11Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-  @transient protected lazy val elementType: DataType =
-    inputTypes.head.asInstanceOf[ArrayType].elementType
+  override lazy val replacement: Expression =
+    ArrayInsert(left, Add(ArraySize(left), Literal(1)), right)


Inspite of ArraySize(left), I think ArrayInsert supports negative values, how about using Literal(-1), to select the position.

Literal(-1) let insert element before the last one.

Oh ! Thanks for confirming!

infoankitp · 2023-03-29T13:46:15Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

@@ -1855,50 +1855,6 @@ class CollectionExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper
    checkEvaluation(ArrayRepeat(Literal("hi"), Literal(null, IntegerType)), null)
  }

-  test("SPARK-41233: ArrayPrepend") {


I hope we have reviewed that we are having all types of test cases deleted here in DataFrameFunctionsSuite. If we are missing, please help add those.

zhengruifeng · 2023-04-01T03:03:19Z

ping @cloud-fan

cloud-fan · 2023-04-03T08:55:15Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-    finalData.update(arrayData.numElements(), elementData)
-    new GenericArrayData(finalData)
-  }
+  override lazy val replacement: Expression = ArrayInsert(left, Literal(0), right)


not related to this PR, but isn't the implementation of array_insert wrong? The doc says: Places val into index pos of array x. Array indices start at 1, or start from the end if index is negative.

According to the doc, array_insert(arr, 1, val) should put val as the first element in this array. array_insert(arr, 0, val) should fail as 0 is not a valid index. However, the current implementation of array_insert uses 0-based index. cc @zhengruifeng @HyukjinKwon @dtenedor

@cloud-fan it is still 1-based, but also treat 0 as the first item. see the discussion #38867 (comment)

+----------------------------------+----------------------------------+----------------------------------+ |array_insert(array(1, 2, 3), 1, 4)|array_insert(array(1, 2, 3), 0, 4)|array_insert(array(1, 2, 3), 2, 4)| +----------------------------------+----------------------------------+----------------------------------+ | [4, 1, 2, 3]| [4, 1, 2, 3]| [1, 4, 2, 3]| +----------------------------------+----------------------------------+----------------------------------+

Sorry for missing that discussion. I think failing is more consistent with other functions that use 1-based index.

And here we should use 1 as the index to be more future-proof.

got it, let me send a fix for it.

If we do want to reserve index 0 for some special behavior in array_insert, I think appending the value to the end of the array seems more convenient?

I chose to remove support for index 0 ...

I think we don't have to reserve index 0, make it fails seems good to me

great, let's fix it soon before the 3.4 release. also cc @xinrong-meng

append value to the end of array for index 0 seems hard to understand.

cloud-fan · 2023-04-03T09:38:07Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

-  @transient protected lazy val elementType: DataType =
-    inputTypes.head.asInstanceOf[ArrayType].elementType
+  override lazy val replacement: Expression =
+    ArrayInsert(left, Add(ArraySize(left), Literal(1)), right)


A risk is left can be evaluated twice if common subexpression elimination is disabled, or the underlying execution engine (native engine) does not support it.

I'd prefer to keep ArrayAppend as it is, or update ArrayInsert to provide an easy way to do append.

### What changes were proposed in this pull request? Make `array_insert` fail when input index `pos` is zero. ### Why are the changes needed? see #40563 (comment) ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? updated UT Closes #40641 from zhengruifeng/sql_array_insert_fails_zero. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Max Gekk <max.gekk@gmail.com>

### What changes were proposed in this pull request? Make `array_insert` fail when input index `pos` is zero. ### Why are the changes needed? see #40563 (comment) ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? updated UT Closes #40641 from zhengruifeng/sql_array_insert_fails_zero. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 3e9574c) Signed-off-by: Max Gekk <max.gekk@gmail.com>

beliefer · 2023-04-04T11:41:20Z

cc @MaxGekk

cloud-fan · 2023-04-04T13:07:43Z

...connect/common/src/test/resources/query-tests/explain-results/function_array_prepend.explain

@@ -1,2 +1,2 @@
-Project [array_prepend(e#0, 1) AS array_prepend(e, 1)#0]
+Project [array_insert(e#0, 0, 1) AS array_prepend(e, 1)#0]


can we fix this?

cloud-fan · 2023-04-04T13:08:37Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala


-  override def nullable: Boolean = left.nullable
+  override lazy val replacement: Expression = ArrayInsert(left, Literal(0), right)


this will fail now.

cloud-fan · 2023-04-06T13:21:50Z

Can we update the PR description? array_append is untouched now.

cloud-fan · 2023-04-06T13:26:08Z

After a second thought, this makes the performance worse. We should improve ArrayInsert to generate more efficient code if the position argument is constant. For example, if the position argument is constant 1, then we don't need to generate code to check if position is negative or not.

beliefer · 2023-04-07T03:31:37Z

Can we update the PR description? array_append is untouched now.

Updated.

beliefer · 2023-04-07T03:32:18Z

After a second thought, this makes the performance worse. We should improve ArrayInsert to generate more efficient code if the position argument is constant. For example, if the position argument is constant 1, then we don't need to generate code to check if position is negative or not.

You means create another PR to simplify the code of ArrayInsert.

…prepend with RuntimeReplaceable

cloud-fan · 2023-04-24T10:29:16Z

thanks, merging to master!

beliefer · 2023-05-04T01:38:31Z

@cloud-fan Thanks !

### What changes were proposed in this pull request? Make `array_insert` fail when input index `pos` is zero. ### Why are the changes needed? see apache#40563 (comment) ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? updated UT Closes apache#40641 from zhengruifeng/sql_array_insert_fails_zero. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 3e9574c) Signed-off-by: Max Gekk <max.gekk@gmail.com>

github-actions bot added the SQL label Mar 27, 2023

beliefer changed the title ~~[SPARK-41232][SPARK-41233][FOLLOWUP] Refactor array_append and array_prepend with RuntimeReplaceable~~ [SPARK-41232][SPARK-41233][FOLLOWUP] Refactor array_append and array_prepend with RuntimeReplaceable Mar 27, 2023

LuciferYang reviewed Mar 27, 2023

View reviewed changes

github-actions bot added the CONNECT label Mar 28, 2023

LuciferYang reviewed Mar 28, 2023

View reviewed changes

navinvishy approved these changes Mar 29, 2023

View reviewed changes

HyukjinKwon reviewed Mar 29, 2023

View reviewed changes

infoankitp reviewed Mar 29, 2023

View reviewed changes

infoankitp approved these changes Mar 30, 2023

View reviewed changes

LuciferYang approved these changes Mar 30, 2023

View reviewed changes

cloud-fan reviewed Apr 3, 2023

View reviewed changes

zhengruifeng mentioned this pull request Apr 3, 2023

[SPARK-43011][SQL] array_insert should fail with 0 index #40641

Closed

cloud-fan reviewed Apr 4, 2023

View reviewed changes

beliefer force-pushed the SPARK-41232_SPARK-41233_followup branch from 2196c2e to b04a175 Compare April 6, 2023 04:57

beliefer changed the title ~~[SPARK-41232][SPARK-41233][FOLLOWUP] Refactor array_append and array_prepend with RuntimeReplaceable~~ [SPARK-41233][FOLLOWUP] Refactor array_prepend with RuntimeReplaceable Apr 6, 2023

cloud-fan approved these changes Apr 6, 2023

View reviewed changes

[SPARK-41232][SPARK-41233][FOLLOWUP] Refactor array_append and array_…

091e63f

…prepend with RuntimeReplaceable

beliefer added 12 commits April 20, 2023 15:14

Update code

005a326

Update code

a3e869a

Update code

3b729e6

Update code

c80f898

Update code

5200f6b

Update collectionOperations.scala

dcd33bc

Update collectionOperations.scala

6213b5a

Update code

44db4ac

Update code

c8737f5

Update code

693f6ec

Update code

06faea8

Update code

c939d6e

beliefer force-pushed the SPARK-41232_SPARK-41233_followup branch from b04a175 to c939d6e Compare April 20, 2023 07:14

cloud-fan closed this in 3e4a32d Apr 24, 2023


		override protected def withNewChildrenInternal(
		newLeft: Expression, newRight: Expression): Expression = {

		@@ -1,2 +1,2 @@
		Project [array_append(e#0, 1) AS array_append(e, 1)#0]
		Project [array_insert(e#0, (size(e#0, false) + 1), 1) AS array_append(e, 1)#0]

		@@ -1,2 +1,2 @@
		Project [array_prepend(e#0, 1) AS array_prepend(e, 1)#0]
		Project [array_insert(e#0, 0, 1) AS array_prepend(e, 1)#0]


		override def nullable: Boolean = left.nullable
		override lazy val replacement: Expression = ArrayInsert(left, Literal(0), right)

[SPARK-41233][FOLLOWUP] Refactor array_prepend with RuntimeReplaceable #40563

[SPARK-41233][FOLLOWUP] Refactor array_prepend with RuntimeReplaceable #40563

Conversation

beliefer commented Mar 27, 2023 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infoankitp Mar 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliefer commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infoankitp Mar 29, 2023 • edited

Choose a reason for hiding this comment

zhengruifeng commented Apr 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliefer commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Apr 6, 2023

cloud-fan commented Apr 6, 2023 • edited

beliefer commented Apr 7, 2023

beliefer commented Apr 7, 2023

cloud-fan commented Apr 24, 2023

beliefer commented May 4, 2023

[SPARK-41233][FOLLOWUP] Refactor `array_prepend` with `RuntimeReplaceable` #40563

[SPARK-41233][FOLLOWUP] Refactor `array_prepend` with `RuntimeReplaceable` #40563

beliefer commented Mar 27, 2023 •

edited

infoankitp Mar 29, 2023 •

edited

infoankitp Mar 29, 2023 •

edited

cloud-fan commented Apr 6, 2023 •

edited