[SPARK-22771][SQL] Concatenate binary inputs into a binary output #19977

maropu · 2017-12-14T08:46:17Z

What changes were proposed in this pull request?

This pr modified concat to concat binary inputs into a single binary output.
concat in the current master always output data as a string. But, in some databases (e.g., PostgreSQL), if all inputs are binary, concat also outputs binary.

How was this patch tested?

Added tests in SQLQueryTestSuite and TypeCoercionSuite.

SparkQA · 2017-12-14T11:36:52Z

Test build #84904 has finished for PR 19977 at commit 8b7dcc9.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-14T15:35:36Z

Test build #84910 has finished for PR 19977 at commit a252bab.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-12-14T18:19:51Z

Could you confirm whether Hive behaves the same?

maropu · 2017-12-14T23:23:44Z

ok

maropu · 2017-12-15T02:25:30Z

checked;

$ hive --version
Hive 2.3.0

hive> create table t1(a string, b string);
hive> create view v1 as select a || b from t1;
hive> describe v1;
OK
_c0                 	string   

hive> create table t2(a binary, b binary);
hive> create view v2 as select a || b from t2;
hive> describe v2;
_c0                 	binary

viirya · 2017-12-15T02:39:32Z

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

+
+  override def inputTypes: Seq[AbstractDataType] =
+    Seq.fill(children.size)(if (isBinaryMode) BinaryType else StringType)
+  override def dataType: DataType = if (isBinaryMode) BinaryType else StringType


Shouldn't we worry about backward compatibility?

yea, should be. Any existing option for keeping back compatibility? Or, how about adding a new option?

Conf is needed for sure. We also need a Migration Guide.

Is it ok to add a new option for this case only? If we keep adding new options for each case, options could blow up?

gatorsmile · 2017-12-15T04:33:53Z

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

@@ -50,15 +51,23 @@ import org.apache.spark.unsafe.types.{ByteArray, UTF8String}
  """)
 case class Concat(children: Seq[Expression]) extends Expression with ImplicitCastInputTypes {

-  override def inputTypes: Seq[AbstractDataType] = Seq.fill(children.size)(StringType)
-  override def dataType: DataType = StringType
+  private lazy val isBinaryMode = children.nonEmpty && children.forall(_.dataType == BinaryType)


If all inputs are binary, concat also outputs binary.

Is this true in Hive and others?

will check some patterns

pg and hive have the same;

postgres=# create table t1(a bytea, b bytea, c varchar, d varchar); postgres=# create view v1 as select a || b || c || d from t1; postgres=# \d v1 View "public.view41_1" Column | Type | Modifiers ----------+------+----------- ?column? | text | hive> create table t1(a binary, b binary, c text, d test); hive> create view v1 as select a || b || c || d from t1; hive> describe v1; _c0 string

Thank you for confirming it! Below is the behavior of DB2

https://www.ibm.com/support/knowledgecenter/en/SSEPGG_11.1.0/com.ibm.db2.luw.sql.ref.doc/doc/r0000781.html

aha, thanks for the info!
I checked the db2 behaviour and I found db2 seems to have a bit different casting rule.
https://www.ibm.com/support/knowledgecenter/SSEPGG_11.1.0/com.ibm.db2.luw.sql.ref.doc/doc/r0000736.html?view=kc
IIUC, in db2, the type of concat(binary, string) is binary?

I also checked mysql: https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_concat

recap:
hive, postgresql: concat(binary, string) => string
mysql, db2: conat(binary, string) => binary

SparkQA · 2017-12-15T08:29:14Z

Test build #84950 has finished for PR 19977 at commit 0d5cd5d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ConcatCoercion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

SparkQA · 2017-12-15T08:59:53Z

Test build #84951 has finished for PR 19977 at commit c3ea879.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

SparkQA · 2017-12-15T12:24:14Z

Test build #84953 has finished for PR 19977 at commit b5fe52c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

SparkQA · 2017-12-15T16:46:50Z

Test build #84963 has finished for PR 19977 at commit ffb6e1c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ConcatCoercion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

SparkQA · 2017-12-16T05:20:08Z

Test build #84987 has finished for PR 19977 at commit 2d3926e.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

maropu · 2017-12-16T05:21:03Z

retest this please

SparkQA · 2017-12-16T08:05:01Z

Test build #84996 has finished for PR 19977 at commit 2d3926e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

maropu · 2017-12-16T08:12:34Z

retest this please

SparkQA · 2017-12-16T11:42:42Z

Test build #85000 has finished for PR 19977 at commit 2d3926e.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

maropu · 2017-12-16T11:49:49Z

oh...

maropu · 2017-12-16T11:49:59Z

retest this please

SparkQA · 2017-12-16T15:16:51Z

Test build #85005 has finished for PR 19977 at commit 2d3926e.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

maropu · 2017-12-17T23:57:01Z

retest this please

SparkQA · 2017-12-18T03:20:40Z

Test build #85031 has finished for PR 19977 at commit 2d3926e.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class Concat(children: Seq[Expression], isBinaryMode: Boolean = false)

gatorsmile · 2017-12-18T08:11:08Z

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

@@ -1035,6 +1035,12 @@ object SQLConf {
    .booleanConf
    .createWithDefault(true)

+  val ConcatBinaryModeEnabled = buildConf("spark.sql.expression.concat.binaryMode.enabled")


~~-> spark.sql.typeCoercion.concatBinaryAsString~~

I'll update after reviews finished

Maybe CONCAT_BINARY_AS_STRING_ENABLED?

-> spark.sql.function.concatBinaryAsString

gatorsmile · 2017-12-18T08:11:41Z

@maropu No need to re-trigger it. The failure is not caused by this PR.

gatorsmile · 2017-12-18T08:11:49Z

Will review it tomorrow. Thanks!

maropu · 2017-12-18T16:54:34Z

I found different behaviours in a string functions elt though, is this expected (binray output is a small surprise?);

hive> create table t(a binary, b binary);
hive> create view v as select elt(1, a, b) from t;
hive> describe v;
_c0                 	string    

mysql> create table t(a binary, b binary);
mysql> create view v as select elt(1, a, b) from t;
mysql> SHOW COLUMNS FROM v;
+--------------+--------------+------+-----+---------+-------+
| Field        | Type         | Null | Key | Default | Extra |
+--------------+--------------+------+-----+---------+-------+
| elt(1, a, b) | varbinary(1) | YES  |     | NULL    |       |
+--------------+--------------+------+-----+---------+-------+
1 row in set (0.00 sec)

scala> val df = Seq(("aaa".getBytes, "bbb".getBytes)).toDF("a", "b")
scala> df.selectExpr("elt(1, a, b)").printSchema
root
 |-- elt(1, CAST(a AS STRING), CAST(b AS STRING)): string (nullable = true)

gcz2022 · 2017-12-19T15:42:46Z

You mean answers of mysql is unexpected? I think it's common these dbs get different behaviors, while Spark mainly follows Hive.

SparkQA · 2017-12-28T06:26:34Z

Test build #85451 has finished for PR 19977 at commit 179c6fd.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-28T06:58:44Z

Test build #85454 has finished for PR 19977 at commit 1c94418.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-12-28T08:40:58Z

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

@@ -24,3 +24,17 @@ select left("abcd", 2), left("abcd", 5), left("abcd", '2'), left("abcd", null);
 select left(null, -2), left("abcd", -2), left("abcd", 0), left("abcd", 'a');
 select right("abcd", 2), right("abcd", 5), right("abcd", '2'), right("abcd", null);
 select right(null, -2), right("abcd", -2), right("abcd", 0), right("abcd", 'a');
+
+-- turn on concatBinaryAsString
+set spark.sql.function.concatBinaryAsString=false;


Since most of other dbms-like systems concat binary inputs as binary, IMO turning off by default is okay to me.

I meant you said turn on in the comment (L28).

viirya · 2017-12-28T08:50:49Z

LGTM

SparkQA · 2017-12-28T17:00:12Z

Test build #85473 has finished for PR 19977 at commit 1e13b70.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-12-29T00:34:04Z

Try

SELECT (col1 || (col3 || col4)) col
FROM (
  SELECT
    string(id) col1,
    encode(string(id + 2), 'utf-8') col3,
    encode(string(id + 3), 'utf-8') col4
  FROM range(10)
)

maropu · 2017-12-29T02:44:31Z

ah, ok. good catch. I'll fix soon.

SparkQA · 2017-12-29T07:57:25Z

Test build #85495 has finished for PR 19977 at commit 57a9d1e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-12-29T10:52:01Z

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

@@ -653,7 +660,11 @@ object CombineConcats extends Rule[LogicalPlan] {
  }

  def apply(plan: LogicalPlan): LogicalPlan = plan.transformExpressionsDown {
-    case concat: Concat if concat.children.exists(_.isInstanceOf[Concat]) =>
+    case concat: Concat if concat.children.exists {


Create a dedicated helper function for the if condition?

SparkQA · 2017-12-29T17:29:21Z

Test build #85511 has finished for PR 19977 at commit b9febbd.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2017-12-30T00:50:44Z

retest this please

SparkQA · 2017-12-30T04:26:24Z

Test build #85535 has finished for PR 19977 at commit b9febbd.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-12-30T06:07:04Z

LGTM

Thanks! Merged to master.

maropu · 2017-12-30T06:27:58Z

thanks, I'll fix elt too in following pr

…tDataTypes ## What changes were proposed in this pull request? This pr is a follow-up to fix a bug left in #19977. ## How was this patch tested? Added tests in `StringExpressionsSuite`. Author: Takeshi Yamamuro <yamamuro@apache.org> Closes #20149 from maropu/SPARK-22771-FOLLOWUP. (cherry picked from commit 6f68316) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

…tDataTypes ## What changes were proposed in this pull request? This pr is a follow-up to fix a bug left in #19977. ## How was this patch tested? Added tests in `StringExpressionsSuite`. Author: Takeshi Yamamuro <yamamuro@apache.org> Closes #20149 from maropu/SPARK-22771-FOLLOWUP.

## What changes were proposed in this pull request? This pr modified `elt` to output binary for binary inputs. `elt` in the current master always output data as a string. But, in some databases (e.g., MySQL), if all inputs are binary, `elt` also outputs binary (Also, this might be a small surprise). This pr is related to #19977. ## How was this patch tested? Added tests in `SQLQueryTestSuite` and `TypeCoercionSuite`. Author: Takeshi Yamamuro <yamamuro@apache.org> Closes #20135 from maropu/SPARK-22937. (cherry picked from commit e8af7e8) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

## What changes were proposed in this pull request? This pr modified `elt` to output binary for binary inputs. `elt` in the current master always output data as a string. But, in some databases (e.g., MySQL), if all inputs are binary, `elt` also outputs binary (Also, this might be a small surprise). This pr is related to #19977. ## How was this patch tested? Added tests in `SQLQueryTestSuite` and `TypeCoercionSuite`. Author: Takeshi Yamamuro <yamamuro@apache.org> Closes #20135 from maropu/SPARK-22937.

viirya reviewed Dec 15, 2017

View reviewed changes

gatorsmile reviewed Dec 15, 2017

View reviewed changes

maropu force-pushed the SPARK-22771 branch from 0d5cd5d to c3ea879 Compare December 15, 2017 08:51

maropu force-pushed the SPARK-22771 branch from c3ea879 to b5fe52c Compare December 15, 2017 09:42

maropu force-pushed the SPARK-22771 branch from b5fe52c to ffb6e1c Compare December 15, 2017 14:00

maropu force-pushed the SPARK-22771 branch from ffb6e1c to 2d3926e Compare December 16, 2017 02:01

gatorsmile reviewed Dec 18, 2017

View reviewed changes

Fix

1c94418

cloud-fan approved these changes Dec 28, 2017

View reviewed changes

viirya reviewed Dec 28, 2017

View reviewed changes

Fix

1e13b70

Fix

57a9d1e

gatorsmile reviewed Dec 29, 2017

View reviewed changes

Fix

b9febbd

asfgit closed this in f2b3525 Dec 30, 2017

This was referenced Jan 2, 2018

[SPARK-22937][SQL] SQL elt output binary for binary inputs #20135

Closed

[SPARK-22771][SQL] Add a missing return statement in Concat.checkInputDataTypes #20149

Closed

[SPARK-22771][SQL] Concatenate binary inputs into a binary output #19977

[SPARK-22771][SQL] Concatenate binary inputs into a binary output #19977

Conversation

maropu commented Dec 14, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Dec 14, 2017

SparkQA commented Dec 14, 2017

gatorsmile commented Dec 14, 2017

maropu commented Dec 14, 2017

maropu commented Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

maropu Dec 15, 2017 • edited Loading

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

SparkQA commented Dec 15, 2017

SparkQA commented Dec 15, 2017

SparkQA commented Dec 15, 2017

SparkQA commented Dec 15, 2017

SparkQA commented Dec 16, 2017

maropu commented Dec 16, 2017

SparkQA commented Dec 16, 2017

maropu commented Dec 16, 2017

SparkQA commented Dec 16, 2017

maropu commented Dec 16, 2017

maropu commented Dec 16, 2017

SparkQA commented Dec 16, 2017

maropu commented Dec 17, 2017

SparkQA commented Dec 18, 2017

gatorsmile Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

maropu Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatorsmile commented Dec 18, 2017

gatorsmile commented Dec 18, 2017

maropu commented Dec 18, 2017 • edited Loading

gcz2022 commented Dec 19, 2017

SparkQA commented Dec 28, 2017

SparkQA commented Dec 28, 2017

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

viirya commented Dec 28, 2017

SparkQA commented Dec 28, 2017

gatorsmile commented Dec 29, 2017

maropu commented Dec 29, 2017

SparkQA commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 29, 2017

maropu commented Dec 30, 2017

SparkQA commented Dec 30, 2017

gatorsmile commented Dec 30, 2017

maropu commented Dec 30, 2017

maropu commented Dec 14, 2017 •

edited

Loading

maropu commented Dec 15, 2017 •

edited

Loading

maropu Dec 15, 2017 •

edited

Loading

gatorsmile Dec 18, 2017 •

edited

Loading

maropu Dec 18, 2017 •

edited

Loading

maropu commented Dec 18, 2017 •

edited

Loading