[SPARK-21054] [SQL] Reset Command support reset specific property. #18268

ericsahit · 2017-06-11T09:21:09Z

What changes were proposed in this pull request?

Reset Command support reset specific property to default value like reset spark.test.property.

Hive 2.1.1 (HIVE-14418) support Reset Command for specific property like reset spark.test.property, which will throw error in SparkSQL.

Compatibility will lower the cost for transferring SQL in production from Hive to SparkSQL.

How was this patch tested?

Add unit tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

ericsahit · 2017-06-11T09:32:40Z

cc @rxin @gatorsmile @cloud-fan Could you please review this? Thanks!

gatorsmile · 2017-06-12T06:45:01Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

No need to mention Hive here. Just need to explain the semantics.

gatorsmile · 2017-06-12T06:49:16Z

ok to test

gatorsmile · 2017-06-12T06:51:02Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

You can use map to shorten it to a single line.

Have been fixed typo except here, @gatorsmile could you please explain a little more? Thanks!

uh... This is a String. Then, we just need to shorten it to a single line

if (raw.nonEmpty) ResetCommand(Some(raw.trim)) else ResetCommand(None)

gatorsmile · 2017-06-12T06:51:18Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

Nit: remove this space

gatorsmile · 2017-06-12T06:53:19Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

Nit: Just use a few lines to implement logics here. No need to add the extra function.

SparkQA · 2017-06-12T06:53:41Z

Test build #77918 has started for PR 18268 at commit 8745848.

SparkQA · 2017-06-13T05:42:36Z

Test build #77965 has started for PR 18268 at commit 9f29f7b.

gatorsmile · 2017-06-13T06:21:41Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

Let us combine the three newly added test cases with the existing ones. For example,

Seq("reset", s"reset ${SQLConf.OPTIMIZER_MAX_ITERATIONS.key}").foreach { cmd => test(s"$cmd - internal conf") { ... sql(cmd) ... }

After done this, building threw SQLConfSuite is not a test, maybe this style does not work. @gatorsmile

cloud-fan · 2017-06-13T07:11:38Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

what about

RESET `special#$!`?

ericsahit · 2017-06-13T08:11:33Z

ok, i have fix typo and code stye according to your comments. @gatorsmile @cloud-fan any suggestion would be welcome. Thanks!

cloud-fan · 2017-06-13T08:11:43Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

actually I was asking about what's hive's behavior for this case...

you mean there has special character in property name? Hive will also take this special#$! as a property name, play same behavior with SparkSQL.
Internally it take rest of command part as property name.

Internally it take rest of command part as property name.

What if there's a space or other punctuation? e.g.reset prop1 prop2;. Hive treats it like one property, two properties, or throws error?

SparkQA · 2017-06-13T09:55:37Z

Test build #77983 has finished for PR 18268 at commit 00db274.

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

SparkQA · 2017-06-13T10:06:58Z

Test build #77984 has finished for PR 18268 at commit 790608f.

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

gatorsmile · 2017-06-14T06:41:23Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

-> test(s"reset - user-defined conf $resetCmd") {

Please do the same for the others.

+1, otherwise we will have tests with same name

cloud-fan · 2017-06-14T08:35:33Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala

can we add a test for

reset `#a!`

ok, now added one plus sql-parse test for reset special character.

…tible with Hive.

SparkQA · 2017-06-15T15:50:35Z

Test build #78106 has finished for PR 18268 at commit 99436d6.

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

ericsahit · 2017-06-15T16:04:52Z

cc @gatorsmile @cloud-fan thanks for reviewing. Any extra suggestion would be welcome

cloud-fan · 2017-06-16T04:18:57Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala

+  test("reset") {
+    assertEqual("reset", ResetCommand(None))
+    assertEqual("reset spark.test.property", ResetCommand(Some("spark.test.property")))
+    assertEqual("reset #$a!", ResetCommand(Some("#$a!")))


can we check hive's behavior? I think special chars are not allowed in config name and parser should throw exception for this case.

Hive 2.1.1 just play the but same behavior. I thought it is due to hardly define what is normal property as user can define property as they wish.
Hive case:

hive (temp)> set #$!@=1024; hive (temp)> set #$!@; #$!@=1024 hive (temp)>

wzhfy · 2017-06-16T05:12:47Z

Hive supports reset multiple keys like: reset config1 config2 or reset config1, config2, should we also support that?

ericsahit · 2017-06-16T11:08:39Z

@gatorsmile @cloud-fan , @wzhfy just give an extra propose, should we support reset multiple property?

I make a hive test like:


hive (temp)> set mapreduce.job.reduces=100;
hive (temp)> set mapreduce.job.reduces;
mapreduce.job.reduces=100
hive (temp)> set mapreduce.job.queuename=root.test;
hive (temp)> set mapreduce.job.queuename;
mapreduce.job.queuename=root.test
hive (temp)> reset mapreduce.job.queuename mapreduce.job.reduces;
hive (temp)> set mapreduce.job.queuename;
mapreduce.job.queuename=root.default
hive (temp)> set mapreduce.job.reduces;
mapreduce.job.reduces=-1

gatorsmile · 2017-06-16T17:13:21Z

Yes. Please do it.

gatorsmile · 2017-06-25T05:57:44Z

ping @ericsahit

SparkQA · 2017-06-25T10:29:14Z

Test build #78576 has finished for PR 18268 at commit ecc2650.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

ericsahit · 2017-06-25T10:33:36Z

@gatorsmile Sorry for the delay, I just add this reset multiple property feature. please review it. Thanks!

And more, fail of unit test seems not related with this patch.

SparkQA · 2017-06-25T10:54:17Z

Test build #78579 has finished for PR 18268 at commit d8e7106.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-06-26T07:44:34Z

retest this please

cloud-fan · 2017-06-26T07:48:52Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

+        assert(spark.conf.get(SQLConf.GROUP_BY_ORDINAL) === true)
+        assert(sql(s"set").where(s"key = '${SQLConf.GROUP_BY_ORDINAL.key}'").count() == 0)
+      } finally {
+        sql(s"set ${SQLConf.GROUP_BY_ORDINAL}=$original")


not introduced in this PR, but we don't need to do this as we call spark.sessionState.conf.clear() at the beginning.

Would you mean it is a duplicated clean action? I suppose the purpose is to prevent impact of other tests..

I think this is OK. After clear(), original gets the default value of the config, so here it wants to keep the default value unchanged.

cloud-fan · 2017-06-26T07:49:58Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

-    } finally {
-      sql(s"set ${SQLConf.GROUP_BY_ORDINAL}=$original")
+  Seq("reset", s"reset ${SQLConf.GROUP_BY_ORDINAL.key}").foreach { resetCmd =>
+    test(s"reset - public conf $resetCmd") {


nit: seems better to make the test name s"$resetCmd - public conf"

done, sounds like a more reasonable name

SparkQA · 2017-06-26T10:07:49Z

Test build #78617 has finished for PR 18268 at commit d8e7106.

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

SparkQA · 2017-06-26T11:31:41Z

Test build #78630 has finished for PR 18268 at commit e4cb39c.

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

cloud-fan · 2017-06-26T12:50:27Z

retest this please

SparkQA · 2017-06-26T14:31:26Z

Test build #78633 has finished for PR 18268 at commit e4cb39c.

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

gatorsmile · 2017-06-27T00:28:53Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

+        sparkSession.sessionState.conf.clear()
+      // "RESET key" clear a specific property.
+      case Some(key) =>
+        key.split("\\s+")


What happened if the space is part of the property key? For example,
RESET `a b` `c d` ?

OK, i plan adding Quoted Identifiers support for such special situation, after all Quote symbol will be the only special delimiter. final use case like this:

spark-sql> set a b=1; key value a b 1 Time taken: 0.018 seconds, Fetched 1 row(s) spark-sql> set c d=2; key value c d 2 Time taken: 0.018 seconds, Fetched 1 row(s) spark-sql> reset a b c d; Time taken: 0.01 seconds spark-sql> set a b; key value a b 1 spark-sql> reset `a b` `c d`; Time taken: 0.01 seconds spark-sql> set a b; key value a b <undefined> Time taken: 0.016 seconds, Fetched 1 row(s) spark-sql> set c d; key value c d <undefined> spark-sql> reset `a b; Error in query: Invalid usage of '`' in expression;

I though we should open another issue,

See the PR #18368

gatorsmile · 2017-08-06T06:25:16Z

ping @ericsahit Could you update your PR with extra changes like #18368 ?

gatorsmile · 2017-10-23T23:56:08Z

ping @ericsahit

AmplabJenkins · 2018-06-09T00:21:25Z

Can one of the admins verify this patch?

gatorsmile reviewed Jun 12, 2017

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala Outdated

Copy link

Member

gatorsmile Jun 12, 2017

Choose a reason for hiding this comment

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

Nit: remove this space

gatorsmile reviewed Jun 12, 2017

View reviewed changes

gatorsmile reviewed Jun 13, 2017

View reviewed changes

cloud-fan reviewed Jun 13, 2017

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala Outdated

Copy link

Contributor

cloud-fan Jun 13, 2017

Choose a reason for hiding this comment

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

what about

RESET `special#$!`?

cloud-fan reviewed Jun 13, 2017

View reviewed changes

gatorsmile reviewed Jun 14, 2017

View reviewed changes

cloud-fan reviewed Jun 14, 2017

View reviewed changes

wanghaihua added 6 commits June 15, 2017 21:22

Add SQL feature of reset spark.test.property support which is compa…

f050f39

…tible with Hive.

Fix typo and add more unit tests.

f2bf940

Fix typo and style.

4e76a31

Fix typo and style according to comment.

579adf9

Fix typo.

cbb424f

Fix and add more test case.

99436d6

ericsahit force-pushed the SPARK-21054 branch from 790608f to 99436d6 Compare June 15, 2017 13:29

cloud-fan reviewed Jun 16, 2017

View reviewed changes

Add reset multiple config feature.

ecc2650

Add comments for use case.

d8e7106

cloud-fan reviewed Jun 26, 2017

View reviewed changes

Modify according to comment.

e4cb39c

gatorsmile reviewed Jun 27, 2017

View reviewed changes

HyukjinKwon mentioned this pull request Jul 16, 2018

[INFRA] Close stale PR #21781

Closed

asfgit closed this in 1a4fda8 Jul 19, 2018

[SPARK-21054] [SQL] Reset Command support reset specific property. #18268

[SPARK-21054] [SQL] Reset Command support reset specific property. #18268

Uh oh!

Conversation

ericsahit commented Jun 11, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

ericsahit commented Jun 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Jun 12, 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!

SparkQA commented Jun 12, 2017

Uh oh!

SparkQA commented Jun 13, 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!

ericsahit commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsahit Jun 13, 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!

wzhfy Jun 13, 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 Jun 13, 2017

Uh oh!

SparkQA commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Jun 14, 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!

ericsahit Jun 15, 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!

SparkQA commented Jun 15, 2017

Uh oh!

ericsahit commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wzhfy commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsahit commented Jun 16, 2017

Uh oh!

gatorsmile commented Jun 16, 2017

ericsahit Jun 13, 2017 •

edited

Loading

wzhfy Jun 13, 2017 •

edited

Loading

gatorsmile Jun 14, 2017 •

edited

Loading

ericsahit Jun 15, 2017 •

edited

Loading

wzhfy commented Jun 16, 2017 •

edited

Loading

ericsahit Jun 26, 2017 •

edited

Loading