[SPARK-37047][SQL][FOLLOWUP] Add legacy flag for the breaking change of lpad and rpad for binary type #36103

anchovYu · 2022-04-07T18:44:08Z

What changes were proposed in this pull request?

Add a legacy flag spark.sql.legacy.lpadRpadAlwaysReturnString for the breaking change introduced in #34154.

The flag is disabled by default. When it is enabled, restore the pre-change behavior that there is no special handling on BINARY input types.

Why are the changes needed?

The original commit is a breaking change, and breaking changes should be encouraged to add a flag to turn it off for smooth migration between versions.

Does this PR introduce any user-facing change?

With the default value of the conf, there is no user-facing difference.
If users turn this conf on, they can restore the pre-change behavior.

How was this patch tested?

Through unit tests.

AmplabJenkins · 2022-04-07T23:45:39Z

Can one of the admins verify this patch?

anchovYu · 2022-04-08T19:40:38Z

Hi @mkaravel @cloud-fan , would you take a look? Thank you!

cloud-fan · 2022-04-11T17:09:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -3709,6 +3709,17 @@ object SQLConf {
      .booleanConf
      .createWithDefault(true)

+  val LPAD_RPAD_FOR_BINARY_TYPE =
+    buildConf("spark.sql.legacy.lpadRpadForBinaryType.enabled")


the config name sounds like it goes to the legacy behavior for binary input if it's set it true. Maybe we should use false as its default value.

I changed it to

val LEGACY_LPAD_RPAD_BINARY_TYPE_AS_STRING = buildConf("spark.sql.legacy.lpadRpadBinaryTypeAsString")

with default value false. Sounds better?

cloud-fan · 2022-04-11T17:10:42Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

@@ -2776,7 +2776,11 @@ object functions {
   * @since 3.3.0
   */
  def lpad(str: Column, len: Int, pad: Array[Byte]): Column = withExpr {
-    BinaryPad("lpad", str.expr, lit(len).expr, lit(pad).expr)


let's just create UnresolvedFunction here.

Do you mean when the legacy is restored, and pad is binary? Why is it a UnresolvedFunction?

By using UnresolveFunction, we will go through the function look up code path and your change in PadExpressionBuilderBase will kick in.

Oh thanks, never know that.

cloud-fan · 2022-04-11T17:11:42Z

sql/core/src/test/resources/sql-tests/inputs/string-functions.sql

@@ -108,6 +111,46 @@ SELECT lpad(x'57', 5, 'abc');
 SELECT rpad('abc', 5, x'57');
 SELECT rpad(x'57', 5, 'abc');

+-- Dsiable the lpad rpad binary breaking change


it's probably an overkill to test the flag like this... I think the change in StringExpressionsSuite is good enough

cloud-fan · 2022-04-11T17:15:14Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala

+        Seq(Literal("hi"), Literal(5), Literal("somepadding")))
+
+      if (SQLConf.get.getConf(SQLConf.LPAD_RPAD_FOR_BINARY_TYPE)) {
+        assert(lpadExp1.asInstanceOf[BinaryPad] ==


nit: we don't need .asInstanceOf[BinaryPad]

cloud-fan · 2022-04-11T17:16:27Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala

+        assert(lpadExp1.asInstanceOf[BinaryPad] ==
+          BinaryPad("lpad", Literal(Array[Byte]()), Literal(5), Literal(Array[Byte](0))))
+        assert(lpadExp2.asInstanceOf[StringLPad] ==
+          StringLPad(Literal("hi"), Literal(5), Literal(" ")))


can we put the ones that have the same result before the if-else? to highlight what gets changed if we flip the config.

cloud-fan · 2022-04-12T01:30:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -3709,6 +3709,18 @@ object SQLConf {
      .booleanConf
      .createWithDefault(true)

+  val LEGACY_LPAD_RPAD_BINARY_TYPE_AS_STRING =
+    buildConf("spark.sql.legacy.lpadRpadBinaryTypeAsString")


how about spark.sql.legacy.lpadRpadAlwaysReturnString? If true, it's the legacy behavior and the pad function always return string even for binary inputs. If false, the function returns binary if the first argument and the padding pattern parameter are both binary type.

Sure! This sounds better. Naming is hard ..

cloud-fan · 2022-04-12T07:21:59Z

sql/core/src/test/resources/sql-tests/inputs/string-functions.sql

@@ -74,6 +74,9 @@ SELECT btrim(encode('xxxbarxxx', 'utf-8'), encode('x', 'utf-8'));
 SELECT lpad('hi', 'invalid_length');
 SELECT rpad('hi', 'invalid_length');

+-- Enable the lpad rpad function for binary input type
+SET spark.sql.legacy.lpadRpadAlwaysReturnString=false;


We don't need to change this sql file

anchovYu · 2022-04-12T18:00:35Z

The test job running: https://github.com/anchovYu/spark/actions/runs/2156689301

cloud-fan · 2022-04-13T05:55:37Z

LGTM, let's update the migration guide to mention this conf

cloud-fan · 2022-04-13T06:25:17Z

thanks, merging to master/3.3 (last commit only changes migration guide)

…of lpad and rpad for binary type ### What changes were proposed in this pull request? Add a legacy flag `spark.sql.legacy.lpadRpadForBinaryType.enabled` for the breaking change introduced in #34154. The flag is enabled by default. When it is disabled, restore the pre-change behavior that there is no special handling on `BINARY` input types. ### Why are the changes needed? The original commit is a breaking change, and breaking changes should be encouraged to add a flag to turn it off for smooth migration between versions. ### Does this PR introduce _any_ user-facing change? With the default value of the conf, there is no user-facing difference. If users turn this conf off, they can restore the pre-change behavior. ### How was this patch tested? Through unit tests. Closes #36103 from anchovYu/flags-lpad-rpad-binary. Authored-by: Xinyi Yu <xinyi.yu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e2683c2) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

add flags and tests for the breaking change

87ecc64

github-actions bot added the SQL label Apr 7, 2022

anchovYu changed the title ~~[WIP][SPARK-37047][SQL][FOLLOWUP] Add legacy flag for the breaking change of lpad and rpad for binary type~~ [SPARK-37047][SQL][FOLLOWUP] Add legacy flag for the breaking change of lpad and rpad for binary type Apr 8, 2022

anchovYu marked this pull request as ready for review April 8, 2022 19:40

cloud-fan reviewed Apr 11, 2022

View reviewed changes

address comments

2330b1c

anchovYu requested a review from cloud-fan April 12, 2022 00:13

cloud-fan reviewed Apr 12, 2022

View reviewed changes

address comments II

1f455c7

anchovYu requested a review from cloud-fan April 12, 2022 06:40

cloud-fan reviewed Apr 12, 2022

View reviewed changes

address final comments

f25a618

empty commit to trigger test

5c50e63

update migration guide

7d33249

cloud-fan approved these changes Apr 13, 2022

View reviewed changes

cloud-fan closed this in e2683c2 Apr 13, 2022

github-actions bot added the DOCS label Apr 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-37047][SQL][FOLLOWUP] Add legacy flag for the breaking change of lpad and rpad for binary type #36103

[SPARK-37047][SQL][FOLLOWUP] Add legacy flag for the breaking change of lpad and rpad for binary type #36103

anchovYu commented Apr 7, 2022 •

edited

AmplabJenkins commented Apr 7, 2022

anchovYu commented Apr 8, 2022

cloud-fan Apr 11, 2022

anchovYu Apr 11, 2022

cloud-fan Apr 11, 2022

anchovYu Apr 11, 2022

cloud-fan Apr 12, 2022

anchovYu Apr 12, 2022

cloud-fan Apr 11, 2022

cloud-fan Apr 11, 2022

cloud-fan Apr 11, 2022

cloud-fan Apr 12, 2022

anchovYu Apr 12, 2022

cloud-fan Apr 12, 2022

anchovYu commented Apr 12, 2022

cloud-fan commented Apr 13, 2022 •

edited

cloud-fan commented Apr 13, 2022

[SPARK-37047][SQL][FOLLOWUP] Add legacy flag for the breaking change of lpad and rpad for binary type #36103

[SPARK-37047][SQL][FOLLOWUP] Add legacy flag for the breaking change of lpad and rpad for binary type #36103

Conversation

anchovYu commented Apr 7, 2022 • 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?

AmplabJenkins commented Apr 7, 2022

anchovYu commented Apr 8, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anchovYu commented Apr 12, 2022

cloud-fan commented Apr 13, 2022 • edited

cloud-fan commented Apr 13, 2022

anchovYu commented Apr 7, 2022 •

edited

cloud-fan commented Apr 13, 2022 •

edited