[SPARK-16285][SQL] Implement sentences SQL functions #14004

dongjoon-hyun · 2016-06-30T22:17:32Z

What changes were proposed in this pull request?

This PR implements sentences SQL function.

How was this patch tested?

Pass the Jenkins tests with a new testcase.

SparkQA · 2016-07-01T02:32:35Z

Test build #61575 has finished for PR 14004 at commit f29a8c3.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sentences(

SparkQA · 2016-07-01T05:13:50Z

Test build #61590 has finished for PR 14004 at commit d5f84c9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sentences(

dongjoon-hyun · 2016-07-01T11:37:15Z

cc @rxin and @cloud-fan

cloud-fan · 2016-07-01T14:13:32Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+  /**
+   * Splits a string into arrays of sentences, where each sentence is an array of words.
+   */
+  public ArrayList<ArrayList<UTF8String>> sentences(UTF8String language, UTF8String country) {


This method is implemented by String not UTF8String, I think we should put it into another util object(maybe in scala) and takes String as arguments.

Oh, I thought it's a convention.

+1. Splitting not-native UTF8String functions sounds good to me. We need to split the followings together.

split
https://github.com/apache/spark/blob/master/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java#L795

translate
https://github.com/apache/spark/blob/master/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java#L805

toTitleCaseSlow
https://github.com/apache/spark/blob/master/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java#L408

For backward compatibility, we need to make Wrappers here?

hmmm, I'm confused too. @rxin what's the convention here?

dongjoon-hyun · 2016-07-02T19:44:45Z

Just rebased.

SparkQA · 2016-07-02T21:42:16Z

Test build #61664 has finished for PR 14004 at commit c9e235a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sentences(

SparkQA · 2016-07-03T08:23:59Z

Test build #61680 has finished for PR 14004 at commit ea75373.

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

dongjoon-hyun · 2016-07-03T17:27:30Z

Rebased to resolve conflicts.

SparkQA · 2016-07-03T19:29:36Z

Test build #61694 has finished for PR 14004 at commit d021d39.

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

cloud-fan · 2016-07-04T04:30:39Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala

+
+    checkEvaluation(
+      Sentences("Hi there! The price was $1,234.56.... But, not now.", "en", "US"),
+        Seq(


wrong ident here

SparkQA · 2016-07-04T07:01:21Z

Test build #61710 has finished for PR 14004 at commit 922e6e7.

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

cloud-fan · 2016-07-06T04:45:13Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+  /**
+   * Return a locale of the given language and country, or a default locale when failures occur.
+   */
+  private Locale getLocale(UTF8String language, UTF8String country) {


I think it's ok to inline this method into sentences

Okay. No problem. I'll move this.

Sorry I mean the sentences method, not the Sentences expression...

dongjoon-hyun · 2016-07-06T05:25:17Z

Now, the PR became more concise. Thank you for decision, @cloud-fan .

dongjoon-hyun · 2016-07-06T05:28:28Z

Oh, @cloud-fan .
Is there some misunderstanding?

dongjoon-hyun · 2016-07-06T05:29:36Z

If then, I will reposition that.
Do you mean making a new java file containing not-really-UTF8String function?

SparkQA · 2016-07-06T05:52:15Z

Test build #61807 has finished for PR 14004 at commit f1a5c1b.

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

cloud-fan · 2016-07-06T06:14:45Z

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

+  def this(str: Expression) = this(str, Literal(""), Literal(""))
+  def this(str: Expression, language: Expression) = this(str, language, Literal(""))
+
+  override def dataType: DataType = ArrayType(ArrayType(StringType))


Is the String element nullable?

Hmm. Right. The return dataType could be null, but has no nullable element. I'll fix like the following.

dongjoon-hyun · 2016-07-06T06:39:23Z

By the way, I found that TernaryExpression.eval seems not to be compatible for sentences('', null, null).
I'll let you know after finishing the update.

SparkQA · 2016-07-06T07:06:40Z

Test build #61816 has finished for PR 14004 at commit 50629b5.

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

SparkQA · 2016-07-06T07:15:16Z

Test build #61818 has finished for PR 14004 at commit a98c05e.

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

dongjoon-hyun · 2016-07-06T09:11:11Z

Hi, @cloud-fan . I've updated the PR. The following is a summary of changes.

To support sentence('', null, null), Sentences extends Expression instead of TerneryExpression.
getLocale is merged into getSentences. (according to the comment.)
getSentences is now in Sentences expression. (Yep. I know. It's accidentaly due to my misunderstanding)
Add more testcases and move test location into StringExpressionsSuite and StringFunctionsSuite.

SparkQA · 2016-07-06T10:15:50Z

Test build #61827 has finished for PR 14004 at commit d2da078.

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

SparkQA · 2016-07-06T10:24:39Z

Test build #61829 has finished for PR 14004 at commit 9e8cfbc.

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

cloud-fan · 2016-07-07T04:00:27Z

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

+    checkEvaluation(
+      Sentences("Hi there! The price was $1,234.56.... But, not now."),
+      correct_answer,
+      EmptyRow)


EmptyRow is the default value, we don't need to pass it.

cloud-fan · 2016-07-07T04:02:59Z

LGTM except some style comment, thanks for working on it!

dongjoon-hyun · 2016-07-07T05:56:46Z

Thank you, @cloud-fan .
I updated the PR according to your comments.

cloud-fan · 2016-07-07T06:33:37Z

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

+      null
+    } else {
+      var locale = Locale.getDefault
+      if (language != null && language.eval(input) != null &&


Usually we don't check the nullability of expression, but the eval result of expression. And it's really bad we call eval twice: one in the if condition and one in the if body.

Oh. I didn't think in that way. Right for both. Thank you.

SparkQA · 2016-07-07T08:00:06Z

Test build #61895 has finished for PR 14004 at commit 8d7c3d4.

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

SparkQA · 2016-07-07T08:55:15Z

Test build #61899 has finished for PR 14004 at commit 4144e7f.

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

dongjoon-hyun · 2016-07-07T09:05:41Z

Hi, @cloud-fan .
Finally, it passed again.

cloud-fan · 2016-07-07T11:52:18Z

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

+      var locale = Locale.getDefault
+      val lang = language.eval(input)
+      val coun = country.eval(input)
+      if (lang != null && coun != null) {


I'd like to write:

val languageStr = language.eval(input).asInstanceOf[UTF8String] val countryStr = country.eval(input).asInstanceOf[UTF8String] val locale = if (languageStr != null && countryStr != null) { new Locale(languageStr, countryStr) } else { Locale.getDefault }

language.eval(input) is null, isn't it?

Oh, it's my mistake. Sorry. I'll fix soon.

cloud-fan · 2016-07-07T11:54:48Z

and one comment for the old thread: #14004 (comment)

SparkQA · 2016-07-07T19:39:00Z

Test build #61920 has finished for PR 14004 at commit 9164f54.

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

dongjoon-hyun · 2016-07-08T06:02:30Z

Hi, @rxin .
Could you review and merge this sentences PR?

rxin · 2016-07-08T06:08:26Z

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

+ * The 'lang' and 'country' arguments are optional, and if omitted, the default locale is used.
+ */
+@ExpressionDescription(
+  usage = "_FUNC_(str, lang, country) - Splits str into an array of array of words.",


_FUNC_(str[, lang, country])

Thank you for review! I'll fix soon.

rxin · 2016-07-08T06:12:24Z

This looks alright. I left some minor comments. Please move this out of the regex file. Seems like it should go into stringExpressions file.

rxin · 2016-07-08T06:37:04Z

LGTM pending Jenkins.

dongjoon-hyun · 2016-07-08T06:38:36Z

Thank you for review again!

SparkQA · 2016-07-08T09:00:33Z

Test build #61969 has finished for PR 14004 at commit 7912bf7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sentences(

dongjoon-hyun · 2016-07-08T09:06:25Z

Hi, @cloud-fan .
Could you merge this sentences PR, too?

dongjoon-hyun · 2016-07-08T09:07:13Z

Oops. You already did. Thank you, @cloud-fan .
And, thank you, @rxin .

## What changes were proposed in this pull request? This PR implements `sentences` SQL function. ## How was this patch tested? Pass the Jenkins tests with a new testcase. Author: Dongjoon Hyun <dongjoon@apache.org> Closes #14004 from dongjoon-hyun/SPARK_16285. (cherry picked from commit a54438c) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2016-07-08T09:09:53Z

thanks, merging to master and 2.0!

cloud-fan reviewed Jul 1, 2016
View reviewed changes

cloud-fan reviewed Jul 4, 2016
View reviewed changes

dongjoon-hyun added 3 commits July 5, 2016 20:22

[SPARK-16285][SQL] Implement sentences SQL functions

41233e6

Add constructors, testcases, and extended description.

d1fb057

Fix indentation and remove unused import.

f1a5c1b

cloud-fan reviewed Jul 6, 2016
View reviewed changes

Address comments.

a98c05e

cloud-fan reviewed Jul 6, 2016
View reviewed changes

Use Expression instead of TernaryExpression.

4529d43

cloud-fan reviewed Jul 7, 2016
View reviewed changes

Address comments.

8d7c3d4

cloud-fan reviewed Jul 7, 2016
View reviewed changes

Remove redundant evaluations.

4144e7f

cloud-fan reviewed Jul 7, 2016
View reviewed changes

Fix the coding style.

9164f54

rxin reviewed Jul 8, 2016
View reviewed changes

Move to stringExpressions and address comments.

7912bf7

asfgit closed this in a54438c Jul 8, 2016

dongjoon-hyun deleted the SPARK_16285 branch July 20, 2016 07:43

[SPARK-16285][SQL] Implement sentences SQL functions #14004

[SPARK-16285][SQL] Implement sentences SQL functions #14004

Conversation

dongjoon-hyun commented Jun 30, 2016

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Jul 1, 2016

SparkQA commented Jul 1, 2016

dongjoon-hyun commented Jul 1, 2016

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

dongjoon-hyun commented Jul 2, 2016

SparkQA commented Jul 2, 2016

SparkQA commented Jul 3, 2016

dongjoon-hyun commented Jul 3, 2016

SparkQA commented Jul 3, 2016

Choose a reason for hiding this comment

SparkQA commented Jul 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jul 6, 2016

dongjoon-hyun commented Jul 6, 2016

dongjoon-hyun commented Jul 6, 2016

SparkQA commented Jul 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jul 6, 2016 • edited

SparkQA commented Jul 6, 2016

SparkQA commented Jul 6, 2016

dongjoon-hyun commented Jul 6, 2016

SparkQA commented Jul 6, 2016

SparkQA commented Jul 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jul 7, 2016

dongjoon-hyun commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 7, 2016

SparkQA commented Jul 7, 2016

dongjoon-hyun commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jul 7, 2016

SparkQA commented Jul 7, 2016

dongjoon-hyun commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxin commented Jul 8, 2016

rxin commented Jul 8, 2016

dongjoon-hyun commented Jul 8, 2016

SparkQA commented Jul 8, 2016

dongjoon-hyun commented Jul 8, 2016

dongjoon-hyun commented Jul 8, 2016

cloud-fan commented Jul 8, 2016

dongjoon-hyun commented Jul 6, 2016 •

edited