SPARK-2407: Added internal implementation of SQL SUBSTR() #1359

willb · 2014-07-10T17:45:04Z

This replaces the Hive UDF for SUBSTR(ING) with an implementation in Catalyst
and adds tests to verify correct operation.

AmplabJenkins · 2014-07-10T17:46:22Z

Merged build started.

AmplabJenkins · 2014-07-10T17:46:30Z

Merged build triggered.

SparkQA · 2014-07-10T17:47:25Z

QA tests have started for PR 1359. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16506/consoleFull

concretevitamin · 2014-07-10T17:52:20Z

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

+
+  def nullable: Boolean = true
+  def dataType: DataType = {
+    if (str.dataType == BinaryType) str.dataType else StringType


Hey Will -- I think we need to check if resolved is true here to not break Catalyst's contract. Similar to here.

concretevitamin · 2014-07-10T18:12:57Z

Hey @willb - thanks for working on this, which is going to be very useful for Spark SQL! I left a couple minor comments. Another general concern is the performance of eval(). If there are ways to reduce branchings, or reduce the function call (not sure if @inline will help), we should probably do it.

willb · 2014-07-10T18:19:26Z

Thanks for the comments, @concretevitamin. I've made the changes from your comments and will think about reducing branch overhead before pushing an update.

willb · 2014-07-10T18:59:50Z

@concretevitamin Inlining Substring.slice seems to make a nontrivial difference (~11.5% speedup) on a very simple Substring.eval() microbenchmark.

concretevitamin · 2014-07-10T19:01:09Z

This sounds pretty cool!

SparkQA · 2014-07-10T19:25:34Z

QA results for PR 1359:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds the following public classes (experimental):
case class Substring(str: Expression, pos: Expression, len: Expression) extends Expression {

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16506/consoleFull

AmplabJenkins · 2014-07-10T19:25:39Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-07-10T19:25:39Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16506/

AmplabJenkins · 2014-07-10T19:41:32Z

Merged build triggered.

AmplabJenkins · 2014-07-10T19:41:37Z

Merged build started.

SparkQA · 2014-07-10T19:42:50Z

QA tests have started for PR 1359. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16513/consoleFull

SparkQA · 2014-07-10T21:18:30Z

QA results for PR 1359:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds the following public classes (experimental):
case class Substring(str: Expression, pos: Expression, len: Expression) extends Expression {

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16513/consoleFull

AmplabJenkins · 2014-07-10T21:18:34Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-07-10T21:18:34Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16513/

marmbrus · 2014-07-11T00:36:50Z

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

+  override def children = str :: pos :: len :: Nil
+
+  @inline
+  def slice[T, C <% IndexedSeqOptimized[T,_]](str: C, startPos: Int, sliceLen: Int): Any = {


Would it be possible to do this without view bounds? I think those are going to disappear soon: https://issues.scala-lang.org/browse/SI-7629

Yes, I think I can do it with implicit parameters. I'll push an update once I've run the Catalyst suite against my change.

SparkQA · 2014-07-11T01:32:35Z

QA tests have started for PR 1359. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16543/consoleFull

SparkQA · 2014-07-11T01:33:15Z

QA results for PR 1359:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds the following public classes (experimental):
case class Substring(str: Expression, pos: Expression, len: Expression) extends Expression {

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16543/consoleFull

SparkQA · 2014-07-11T01:37:28Z

QA tests have started for PR 1359. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16544/consoleFull

SparkQA · 2014-07-11T03:13:34Z

QA results for PR 1359:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds the following public classes (experimental):
case class Substring(str: Expression, pos: Expression, len: Expression) extends Expression {

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16544/consoleFull

marmbrus · 2014-07-14T22:37:17Z

Hey Will, this looks great. Thanks for implementing! Mind fixing the conflict with master so I can merge?

This replaces the Hive UDF for SUBSTR(ING) with an implementation in Catalyst and adds tests to verify correct operation. Squashes 8969d038 and e6419b4e.

@concretevitamin

* orders imports in stringOperations.scala * Substring.dataType throws exception if children are unresolved * inlines Substring.slice (~11.5% performance improvement on microbenchmark runs) * adds a special `toString` case for two-argument SUBSTR expressions * removes spurious I_ prefix to SUBSTR(ING) in HiveQL.scala Thanks to @concretevitamin for prompt and useful feedback!

marmbrus · 2014-07-15T03:32:57Z

add to whitelist

SparkQA · 2014-07-15T03:37:48Z

QA tests have started for PR 1359. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16652/consoleFull

SparkQA · 2014-07-15T05:14:31Z

QA results for PR 1359:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds the following public classes (experimental):
case class Substring(str: Expression, pos: Expression, len: Expression) extends Expression {

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16652/consoleFull

marmbrus · 2014-07-15T21:10:58Z

I'm going to go ahead and merge this. Someone can make a follow-up with nullability narrowing.

marmbrus · 2014-07-15T21:12:33Z

Thanks! Merged into master and 1.0.

This replaces the Hive UDF for SUBSTR(ING) with an implementation in Catalyst and adds tests to verify correct operation. Author: William Benton <[email protected]> Closes #1359 from willb/internalSqlSubstring and squashes the following commits: ccedc47 [William Benton] Fixed too-long line. a30a037 [William Benton] replace view bounds with implicit parameters ec35c80 [William Benton] Adds fixes from review: 4f3bfdb [William Benton] Added internal implementation of SQL SUBSTR() (cherry picked from commit 61de65b) Signed-off-by: Michael Armbrust <[email protected]>

This is a follow-up of #1359 with nullability narrowing. Author: Takuya UESHIN <[email protected]> Closes #1426 from ueshin/issues/SPARK-2504 and squashes the following commits: 5157832 [Takuya UESHIN] Remove unnecessary white spaces. 80958ac [Takuya UESHIN] Fix nullability of Substring expression. (cherry picked from commit 632fb3d) Signed-off-by: Reynold Xin <[email protected]>

This is a follow-up of #1359 with nullability narrowing. Author: Takuya UESHIN <[email protected]> Closes #1426 from ueshin/issues/SPARK-2504 and squashes the following commits: 5157832 [Takuya UESHIN] Remove unnecessary white spaces. 80958ac [Takuya UESHIN] Fix nullability of Substring expression.

chutium · 2014-07-16T11:53:20Z

hi, it is really very useful for us, i tried this implementation from @willb , in spark-shell, i still got java.lang.UnsupportedOperationException by Query Plan, i made some change in SqlParser: chutium@1de83a7

marmbrus · 2014-07-16T11:58:37Z

Awesome! Please submit a pull request with that addition.
On Jul 16, 2014 7:53 AM, "Teng Qiu" [email protected] wrote:

hi, it is really very useful for us, i tried this implementation from
@willb https://github.com/willb , in spark-shell, i still got
java.lang.UnsupportedOperationException by Query Plan, i made some change
in SqlParser: chutium/spark@1de83a7
chutium@1de83a7

—
Reply to this email directly or view it on GitHub
#1359 (comment).

chutium · 2014-07-16T12:45:39Z

PR submitted #1442

follow-up of #1359 Author: chutium <[email protected]> Closes #1442 from chutium/master and squashes the following commits: b49cc8a [chutium] SPARK-2407: Added Parser of SQL SUBSTRING() #1442 9a60ccf [chutium] SPARK-2407: Added Parser of SQL SUBSTR() #1442 06e933b [chutium] Merge https://github.com/apache/spark c870172 [chutium] Merge https://github.com/apache/spark 094f773 [chutium] Merge https://github.com/apache/spark 88cb37d [chutium] Merge https://github.com/apache/spark 1de83a7 [chutium] SPARK-2407: Added Parse of SQL SUBSTR()

This replaces the Hive UDF for SUBSTR(ING) with an implementation in Catalyst and adds tests to verify correct operation. Author: William Benton <[email protected]> Closes apache#1359 from willb/internalSqlSubstring and squashes the following commits: ccedc47 [William Benton] Fixed too-long line. a30a037 [William Benton] replace view bounds with implicit parameters ec35c80 [William Benton] Adds fixes from review: 4f3bfdb [William Benton] Added internal implementation of SQL SUBSTR()

This is a follow-up of apache#1359 with nullability narrowing. Author: Takuya UESHIN <[email protected]> Closes apache#1426 from ueshin/issues/SPARK-2504 and squashes the following commits: 5157832 [Takuya UESHIN] Remove unnecessary white spaces. 80958ac [Takuya UESHIN] Fix nullability of Substring expression.

follow-up of apache#1359 Author: chutium <[email protected]> Closes apache#1442 from chutium/master and squashes the following commits: b49cc8a [chutium] SPARK-2407: Added Parser of SQL SUBSTRING() apache#1442 9a60ccf [chutium] SPARK-2407: Added Parser of SQL SUBSTR() apache#1442 06e933b [chutium] Merge https://github.com/apache/spark c870172 [chutium] Merge https://github.com/apache/spark 094f773 [chutium] Merge https://github.com/apache/spark 88cb37d [chutium] Merge https://github.com/apache/spark 1de83a7 [chutium] SPARK-2407: Added Parse of SQL SUBSTR()

concretevitamin reviewed Jul 10, 2014
View reviewed changes

marmbrus reviewed Jul 11, 2014
View reviewed changes

willb added 2 commits July 14, 2014 17:55

Added internal implementation of SQL SUBSTR()

4f3bfdb

This replaces the Hive UDF for SUBSTR(ING) with an implementation in Catalyst and adds tests to verify correct operation. Squashes 8969d038 and e6419b4e.

asfgit closed this in 61de65b Jul 15, 2014

ueshin mentioned this pull request Jul 16, 2014

[SPARK-2504][SQL] Fix nullability of Substring expression. #1426

Closed

chutium mentioned this pull request Jul 16, 2014

SPARK-2407: Added Parser of SQL SUBSTR() #1442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPARK-2407: Added internal implementation of SQL SUBSTR() #1359

SPARK-2407: Added internal implementation of SQL SUBSTR() #1359

willb commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

SparkQA commented Jul 10, 2014

concretevitamin Jul 10, 2014

concretevitamin commented Jul 10, 2014

willb commented Jul 10, 2014

willb commented Jul 10, 2014

concretevitamin commented Jul 10, 2014

SparkQA commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

SparkQA commented Jul 10, 2014

SparkQA commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

marmbrus Jul 11, 2014

willb Jul 11, 2014

SparkQA commented Jul 11, 2014

SparkQA commented Jul 11, 2014

SparkQA commented Jul 11, 2014

SparkQA commented Jul 11, 2014

marmbrus commented Jul 14, 2014

marmbrus commented Jul 15, 2014

SparkQA commented Jul 15, 2014

SparkQA commented Jul 15, 2014

marmbrus commented Jul 15, 2014

marmbrus commented Jul 15, 2014

chutium commented Jul 16, 2014

marmbrus commented Jul 16, 2014

chutium commented Jul 16, 2014

SPARK-2407: Added internal implementation of SQL SUBSTR() #1359

SPARK-2407: Added internal implementation of SQL SUBSTR() #1359

Conversation

willb commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

SparkQA commented Jul 10, 2014

concretevitamin Jul 10, 2014

Choose a reason for hiding this comment

concretevitamin commented Jul 10, 2014

willb commented Jul 10, 2014

willb commented Jul 10, 2014

concretevitamin commented Jul 10, 2014

SparkQA commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

SparkQA commented Jul 10, 2014

SparkQA commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

AmplabJenkins commented Jul 10, 2014

marmbrus Jul 11, 2014

Choose a reason for hiding this comment

willb Jul 11, 2014

Choose a reason for hiding this comment

SparkQA commented Jul 11, 2014

SparkQA commented Jul 11, 2014

SparkQA commented Jul 11, 2014

SparkQA commented Jul 11, 2014

marmbrus commented Jul 14, 2014

marmbrus commented Jul 15, 2014

SparkQA commented Jul 15, 2014

SparkQA commented Jul 15, 2014

marmbrus commented Jul 15, 2014

marmbrus commented Jul 15, 2014

chutium commented Jul 16, 2014

marmbrus commented Jul 16, 2014

chutium commented Jul 16, 2014