[SPARK-21007][SQL]Add SQL function - RIGHT && LEFT #18228

10110346 · 2017-06-07T08:37:14Z

What changes were proposed in this pull request?

Add SQL function - RIGHT && LEFT, same as MySQL:
https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_left
https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_right

How was this patch tested?

unit test

sameeragarwal · 2017-06-08T22:38:06Z

ok to test

sameeragarwal · 2017-06-08T22:38:14Z

jenkins add to whitelist

SparkQA · 2017-06-09T00:56:44Z

Test build #77824 has finished for PR 18228 at commit e227528.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression)

rxin · 2017-06-10T01:43:38Z

Are these ANSI SQL functions? If it is just some esoteric MySQL function I don't think we should add them.

10110346 · 2017-06-10T07:16:23Z

Both of mysql and SQL server support these two functions, oracle don't support these functions.

SparkQA · 2017-06-15T10:22:32Z

Test build #78090 has finished for PR 18228 at commit 2136c1b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression)

gatorsmile · 2017-06-19T01:42:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala

It can be implemented using RuntimeReplaceable. For example, NullIf

SparkQA · 2017-06-22T06:17:14Z

Test build #78422 has finished for PR 18228 at commit fca984d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression)
case class Left(str: Expression, len: Expression)

SparkQA · 2017-06-22T09:25:55Z

Test build #78437 has finished for PR 18228 at commit 9b85478.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression)
case class Left(str: Expression, len: Expression)

viirya · 2017-07-04T03:26:22Z

As we already have substring, do we still need them?

10110346 · 2017-07-04T03:39:39Z

Substring is the most commonly used as left and right, and i think using these form is more friendly for users.
Also mysql and SQL server support these two functions and Substring .Thanks @viirya

gatorsmile · 2017-07-07T23:53:36Z

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

Please change it to something like

val pos = xyz val len = xyz string.asInstanceOf[UTF8String].substringSQL(pos, len)

gatorsmile · 2017-07-07T23:54:43Z

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

Do not split the codes in the middle.

gatorsmile · 2017-07-08T00:05:47Z

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

Could you post the outputs of MySQL when the length is negative?

10110346 · 2017-07-10T01:29:04Z

@gatorsmile Thanks
I have tested in mysql:

SparkQA · 2017-07-10T05:06:47Z

Test build #79432 has finished for PR 18228 at commit 2eb4d0f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression)
case class Left(str: Expression, len: Expression)

cloud-fan · 2017-07-10T06:02:58Z

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

shall we use RuntimeReplaceable? I think both left and right can be implemented by substring

ok,i will do,thanks

Left has been implemented by substring, but Right implemented by substring may be not very good:

case class Right(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable { def this(str: Expression, len: Expression) = { this(str, len, Substring(str, If(LessThanOrEqual(len, Literal(0)), Literal(Integer.MAX_VALUE), UnaryMinus(len)), len)) } override def flatArguments: Iterator[Any] = Iterator(str, len) override def sql: String = s"$prettyName(${str.sql}, ${len.sql})" }

Substring supports negative position, we can implement Right as Substring(str, UnaryMinus(len)).

For example:
select right("sparksql",-2);
for this case,we expected is "",
if we implement Right as Substring(str, UnaryMinus(len)). this result will be parksql

ok so we should do: If(LessThanOrEqual(len, Literal(0), Literal(UTF8String.EMPTY_UTF8), Substring(str, UnaryMinus(len))). Complex expression is OK, after codegen, it should be almost same as a customize implementation.

OK,thank you very much.

SparkQA · 2017-07-10T10:41:13Z

Test build #79456 has finished for PR 18228 at commit 4a7941e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression)
case class Left(str: Expression, len: Expression, child: Expression)

cloud-fan · 2017-07-11T07:22:08Z

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

shall we also explain the behavior if len is less or equal than 0?

also mention that len can be string type. BTW is this common in other databases to support string type len?

Yes, MYSQL support string type len, too

cloud-fan · 2017-07-11T07:23:03Z

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

cloud-fan · 2017-07-11T07:25:12Z

sql/core/src/test/resources/sql-tests/results/string-functions.sql.out

is this the corrected answer?

Maybe this has a question with this test case: left("abcd", 'a')
In Mysql:
mysql> select left("abcd", -2), left("abcd", 0), left("abcd", 'a');
+------------------+-----------------+-------------------+
| left("abcd", -2) | left("abcd", 0) | left("abcd", 'a') |
+------------------+-----------------+-------------------+
| | | |
+------------------+-----------------+-------------------+
mysql> select right("abcd", -2), right("abcd", 0), right("abcd", 'a');
+-------------------+------------------+--------------------+
| right("abcd", -2) | right("abcd", 0) | right("abcd", 'a') |
+-------------------+------------------+--------------------+
| | | |
+-------------------+------------------+--------------------+
Substring is same as Left

SparkQA · 2017-07-11T09:47:53Z

Test build #79510 has finished for PR 18228 at commit fa81e44.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression, child: Expression)
case class Left(str: Expression, len: Expression, child: Expression)

SparkQA · 2017-07-11T09:53:21Z

Test build #79511 has finished for PR 18228 at commit 1cb0448.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable
case class Left(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable

SparkQA · 2017-07-11T12:34:20Z

Test build #79518 has finished for PR 18228 at commit 529417f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable
case class Left(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable

cloud-fan · 2017-07-11T14:14:37Z

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

int: ... the result is an empty string

cloud-fan · 2017-07-11T14:17:00Z

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

I prefer If(LessThanOrEqual(len, Literal(0), Literal(UTF8String.EMPTY_UTF8), Substring(str, UnaryMinus(len))).

The reason is that, your expression will end up calling UTF8String.substringSQL(Int.Max, ...), which goes through all bytes in this UTF8String and is a performance waste.

right(null, -10)
I agree with you, but , for this test case, there is a problem:
Which we expected is null,but it is an empty string

cloud-fan · 2017-07-12T03:35:17Z

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

we can do the null check first, e.g.

If( IsNull(str), Literal(null, StringType), If( LessThanOrEqual(len, Literal(0)), Literal(UTF8String.EMPTY_UTF8, StringType), new Substring(str, UnaryMinus(len)) ) )

SparkQA · 2017-07-12T05:22:32Z

Test build #79548 has finished for PR 18228 at commit b75b6ac.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable
case class Left(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable

SparkQA · 2017-07-12T07:04:55Z

Test build #79550 has finished for PR 18228 at commit 991bf99.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable
case class Left(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable

10110346 · 2017-07-12T07:11:12Z

retest this please

SparkQA · 2017-07-12T09:46:20Z

Test build #79551 has finished for PR 18228 at commit 991bf99.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Right(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable
case class Left(str: Expression, len: Expression, child: Expression) extends RuntimeReplaceable

cloud-fan · 2017-07-12T10:51:41Z

LGTM, merging to master!

10110346 force-pushed the lx-wip-0607 branch 7 times, most recently from 128d3e5 to e227528 Compare June 8, 2017 04:30

10110346 force-pushed the lx-wip-0607 branch 2 times, most recently from 141a42f to 2136c1b Compare June 15, 2017 08:04

gatorsmile reviewed Jun 19, 2017

View reviewed changes

10110346 force-pushed the lx-wip-0607 branch from 2136c1b to fca984d Compare June 22, 2017 03:51

10110346 force-pushed the lx-wip-0607 branch from fca984d to 9b85478 Compare June 22, 2017 07:00

gatorsmile reviewed Jul 7, 2017

View reviewed changes

gatorsmile reviewed Jul 8, 2017

View reviewed changes

10110346 force-pushed the lx-wip-0607 branch from 9b85478 to 2eb4d0f Compare July 10, 2017 02:41

cloud-fan reviewed Jul 10, 2017

View reviewed changes

10110346 force-pushed the lx-wip-0607 branch from 2eb4d0f to 4a7941e Compare July 10, 2017 08:17

10110346 force-pushed the lx-wip-0607 branch 2 times, most recently from fa81e44 to 1cb0448 Compare July 11, 2017 07:21

cloud-fan reviewed Jul 11, 2017

View reviewed changes

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

Copy link

Contributor

cloud-fan Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

cloud-fan reviewed Jul 11, 2017

View reviewed changes

10110346 force-pushed the lx-wip-0607 branch from 1cb0448 to 529417f Compare July 11, 2017 10:04

cloud-fan reviewed Jul 11, 2017

View reviewed changes

10110346 force-pushed the lx-wip-0607 branch from 529417f to b75b6ac Compare July 12, 2017 02:45

cloud-fan reviewed Jul 12, 2017

View reviewed changes

Add SQL function - RIGHT && LEFT

991bf99

10110346 force-pushed the lx-wip-0607 branch from b75b6ac to 991bf99 Compare July 12, 2017 05:29

asfgit closed this in aaad34d Jul 12, 2017

[SPARK-21007][SQL]Add SQL function - RIGHT && LEFT #18228

[SPARK-21007][SQL]Add SQL function - RIGHT && LEFT #18228

Uh oh!

Conversation

10110346 commented Jun 7, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

sameeragarwal commented Jun 8, 2017

Uh oh!

sameeragarwal commented Jun 8, 2017

Uh oh!

SparkQA commented Jun 9, 2017

Uh oh!

rxin commented Jun 10, 2017

Uh oh!

10110346 commented Jun 10, 2017

Uh oh!

SparkQA commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 22, 2017

Uh oh!

SparkQA commented Jun 22, 2017

Uh oh!

viirya commented Jul 4, 2017

Uh oh!

10110346 commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

10110346 commented Jul 10, 2017

Uh oh!

SparkQA commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

10110346 Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

10110346 Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 11, 2017

Uh oh!

SparkQA commented Jul 11, 2017

Uh oh!

SparkQA commented Jul 11, 2017

10110346 commented Jul 4, 2017 •

edited

Loading

10110346 Jul 10, 2017 •

edited

Loading

10110346 Jul 11, 2017 •

edited

Loading

cloud-fan Jul 11, 2017 •

edited

Loading