[SPARK-12982][SQL] Add table name validation in temp table registration #11051

jayadevanmurali · 2016-02-03T14:38:42Z

Add the table name validation at the temp table creation

Update from original

Add table name validation in temp table registration

Added test case for validating table name

hvanhovell · 2016-02-03T17:37:00Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

@@ -1291,4 +1291,14 @@ class DataFrameSuite extends QueryTest with SharedSQLContext {
      Seq(1 -> "a").toDF("i", "j").filter($"i".cast(StringType) === "1"),
      Row(1, "a"))
  }
+
+  test("SPARK-12982: Add table name validation in temp table registration") {
+    val rows = List(Row("foo"), Row("bar"))


We can be a bit more concise: val df = Seq("foo", "bar").map(Tuple1.apply).toDF("col")

@hvanhovell OK I will do the change

davies · 2016-02-03T18:36:15Z

Jenkins, OK to test

hvanhovell · 2016-02-04T14:01:41Z

ok to test

hvanhovell · 2016-02-04T15:37:32Z

@davies can you trigger a test?

SparkQA · 2016-02-04T17:04:50Z

Test build #2516 has finished for PR 11051 at commit 3407fd7.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

Removed extra white space.

jayadevanmurali · 2016-02-04T17:49:00Z

@SparkQA Could you please test again

SparkQA · 2016-02-04T19:20:58Z

Test build #2517 has finished for PR 11051 at commit 4c25f2e.

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

Modified test case by including more valid and invalid table names

jayadevanmurali · 2016-02-08T08:59:42Z

@SparkQA
Could you please do a test again

@hvanhovell @davies
Incorporated review comments in test case. Could you please review the same

hvanhovell · 2016-02-08T09:11:24Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

@@ -1291,4 +1291,19 @@ class DataFrameSuite extends QueryTest with SharedSQLContext {
      Seq(1 -> "a").toDF("i", "j").filter($"i".cast(StringType) === "1"),
      Row(1, "a"))
  }
+
+  test("SPARK-12982: Add table name validation in temp table registration") {
+    val rows = List(Row("foo"), Row("bar"))


We can be a bit more concise (also see the comment in the previous PR): val df = Seq(("one", "two")).toDF("a", "b")

hvanhovell · 2016-02-08T09:11:38Z

retest this please

jayadevanmurali · 2016-02-08T09:12:56Z

@hvanhovell
I will make that change now.

hvanhovell · 2016-02-08T09:13:56Z

@jayadevanmurali you can request a retest by making a retest this please comment. The Jenkins PR builder will pick this up. See https://github.com/jenkinsci/ghprb-plugin/blob/master/README.md for more information.

jayadevanmurali · 2016-02-08T09:15:42Z

Thanks @hvanhovell for this info

Incorporated review comments, Make the code more concise

jayadevanmurali · 2016-02-08T09:23:39Z

retest this please

hvanhovell · 2016-02-08T10:26:45Z

LGTM pending tests

SparkQA · 2016-02-08T17:58:16Z

Test build #2523 has finished for PR 11051 at commit d576496.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2016-02-08T22:15:10Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

+
+  test("SPARK-12982: Add table name validation in temp table registration") {
+    val df = Seq("foo", "bar").map(Tuple1.apply).toDF("col")
+    //invalid table name test as below


You need to add a space between // and the actual comment in order to pass style checks.

Insert one white space at the beginning of the comment.

jayadevanmurali · 2016-02-09T04:16:45Z

Jenkins, retest this please

hvanhovell · 2016-02-09T08:22:47Z

retest this please

jayadevanmurali · 2016-02-09T17:01:04Z

@SparkQA
Could you please retest the changes

hvanhovell · 2016-02-09T21:08:41Z

retest this please

jayadevanmurali · 2016-02-10T16:16:12Z

@hvanhovell @davies
Why this much delay for test ? Is any action required from me?

jayadevanmurali · 2016-02-10T16:27:58Z

retest this please

hvanhovell · 2016-02-10T17:59:45Z

retest this please

hvanhovell · 2016-02-10T19:19:36Z

@jayadevanmurali I have no idea why builds aren't triggered (they should be). I haven't got the proper Jenkins rights to trigger a build yet.

hvanhovell · 2016-02-10T19:20:23Z

ok to test

SparkQA · 2016-02-10T20:40:58Z

Test build #2530 has finished for PR 11051 at commit f3c7bbb.

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

hvanhovell · 2016-02-10T21:15:13Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

+    // another invalid table name test as below
+    intercept[AnalysisException](df.registerTempTable("#$@sum"))
+    // another invalid table name test as below
+    intercept[AnalysisException](df.registerTempTable("table!#"))


Hmmmm... this one should fail. Let me submit a PR for this.

Is this a new bug ?

It is a bug, but it was in code base already. The fix is trivial (I need to add a separate rule for parsing table identifiers - which checks for an EOF).

Ok thanks for the info. Let me know if you need any help from me.

…xpression string The parser currently parses the following strings without a hitch: * Table Identifier: * `a.b.c` should fail, but results in the following table identifier `a.b` * `table!#` should fail, but results in the following table identifier `table` * Expression * `1+2 r+e` should fail, but results in the following expression `1 + 2` This PR fixes this by adding terminated rules for both expression parsing and table identifier parsing. cc cloud-fan (we discussed this in #10649) jayadevanmurali (this causes your PR #11051 to fail) Author: Herman van Hovell <hvanhovell@questtec.nl> Closes #11159 from hvanhovell/SPARK-13276.

jayadevanmurali · 2016-02-11T08:22:44Z

@hvanhovell Thanks for the quick fix, I think we can retest this PR now. What you think ?

hvanhovell · 2016-02-11T13:32:16Z

retest this please

jayadevanmurali · 2016-02-11T14:23:58Z

@hvanhovell I think, you may comment "ok to test" for triggering the build

hvanhovell · 2016-02-11T14:40:01Z

ok to test

hvanhovell · 2016-02-11T14:42:39Z

@jayadevanmurali issueing retest this please should normally do the trick.

hvanhovell · 2016-02-11T15:38:19Z

@jayadevanmurali Jenkins will be restarted in 30 mins. Lets try again after that.

hvanhovell · 2016-02-11T16:22:50Z

retest this please

jayadevanmurali · 2016-02-11T16:24:45Z

Ok. I think something missing in our test request, lot test started now which all requested after our PR

jayadevanmurali · 2016-02-11T17:18:14Z

Jenkins, test this please

hvanhovell · 2016-02-11T17:18:29Z

ok to test

hvanhovell · 2016-02-11T17:21:53Z

@shaneknapp any idea why Jenkins is not picking this up?

shaneknapp · 2016-02-11T17:27:19Z

jenkins test this please

shaneknapp · 2016-02-11T17:28:41Z

im not totally sure... jenkins is picking up other PRB builds. i'll poke around the (horrific) jenkins logs and see if i can't find out.

shaneknapp · 2016-02-11T17:34:27Z

it's building now:

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/51120/

hvanhovell · 2016-02-11T19:06:02Z

@shaneknapp thanks for your help! Any idea what caused this?

SparkQA · 2016-02-11T19:06:56Z

Test build #51120 has finished for PR 11051 at commit f3c7bbb.

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

shaneknapp · 2016-02-11T19:17:30Z

the GHPRB plugin is finicky, obtuse and opaque in it's operations. i have
no idea why it decided to not trigger, and the logs don't show anything.
sorry... :(

On Thu, Feb 11, 2016 at 11:07 AM, Herman van Hovell <
notifications@github.com> wrote:

@shaneknapp https://github.com/shaneknapp thanks for your help! Any
idea what caused this?

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

hvanhovell · 2016-02-11T20:20:09Z

@shaneknapp Thanks for checking... The GHPRB just works in mysterious ways...

hvanhovell · 2016-02-11T20:21:43Z

Merging to master. Thanks!

jayadevanmurali added 3 commits January 31, 2016 07:58

Merge pull request #1 from apache/master

290b86b

Update from original

Update SQLContext.scala

7c7b503

Add table name validation in temp table registration

Update DataFrameSuite.scala

3407fd7

Added test case for validating table name

jayadevanmurali mentioned this pull request Feb 3, 2016

[SPARK-12982][SQL] Add table name validation in temp table registration #10983

Closed

hvanhovell reviewed Feb 3, 2016
View reviewed changes

Update DataFrameSuite.scala

4c25f2e

Removed extra white space.

Update DataFrameSuite.scala

a9e3b32

Modified test case by including more valid and invalid table names

hvanhovell reviewed Feb 8, 2016
View reviewed changes

Update DataFrameSuite.scala

d576496

Incorporated review comments, Make the code more concise

hvanhovell reviewed Feb 8, 2016
View reviewed changes

Update DataFrameSuite.scala

f3c7bbb

Insert one white space at the beginning of the comment.

hvanhovell reviewed Feb 10, 2016
View reviewed changes

hvanhovell mentioned this pull request Feb 10, 2016

[SPARK-13276] Catch bad characters at the end of a Table Identifier/Expression string #11159

Closed

asfgit closed this in 0d50a22 Feb 11, 2016

[SPARK-12982][SQL] Add table name validation in temp table registration #11051

[SPARK-12982][SQL] Add table name validation in temp table registration #11051

Conversation

jayadevanmurali commented Feb 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davies commented Feb 3, 2016

hvanhovell commented Feb 4, 2016

hvanhovell commented Feb 4, 2016

SparkQA commented Feb 4, 2016

jayadevanmurali commented Feb 4, 2016

SparkQA commented Feb 4, 2016

jayadevanmurali commented Feb 8, 2016

Choose a reason for hiding this comment

hvanhovell commented Feb 8, 2016

jayadevanmurali commented Feb 8, 2016

hvanhovell commented Feb 8, 2016

jayadevanmurali commented Feb 8, 2016

jayadevanmurali commented Feb 8, 2016

hvanhovell commented Feb 8, 2016

SparkQA commented Feb 8, 2016

Choose a reason for hiding this comment

jayadevanmurali commented Feb 9, 2016

hvanhovell commented Feb 9, 2016

jayadevanmurali commented Feb 9, 2016

hvanhovell commented Feb 9, 2016

jayadevanmurali commented Feb 10, 2016

jayadevanmurali commented Feb 10, 2016

hvanhovell commented Feb 10, 2016

hvanhovell commented Feb 10, 2016

hvanhovell commented Feb 10, 2016

SparkQA commented Feb 10, 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

jayadevanmurali commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

jayadevanmurali commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

jayadevanmurali commented Feb 11, 2016

jayadevanmurali commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

shaneknapp commented Feb 11, 2016

shaneknapp commented Feb 11, 2016

shaneknapp commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

SparkQA commented Feb 11, 2016

shaneknapp commented Feb 11, 2016

hvanhovell commented Feb 11, 2016

hvanhovell commented Feb 11, 2016