Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ericsahit wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f050f39
Add SQL feature of `reset spark.test.property` support which is compa…
Jun 9, 2017
f2bf940
Fix typo and add more unit tests.
Jun 11, 2017
4e76a31
Fix typo and style.
Jun 13, 2017
579adf9
Fix typo and style according to comment.
Jun 13, 2017
cbb424f
Fix typo.
Jun 13, 2017
99436d6
Fix and add more test case.
Jun 15, 2017
ecc2650
Add reset multiple config feature.
Jun 25, 2017
d8e7106
Add comments for use case.
Jun 25, 2017
e4cb39c
Modify according to comment.
Jun 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix and add more test case.
  • Loading branch information
wanghaihua committed Jun 15, 2017
commit 99436d624d028fd972d5ec94eaedcf1b639b23a7
1 change: 1 addition & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,5 +305,6 @@ class SparkSqlParserSuite extends PlanTest {
test("reset") {
assertEqual("reset", ResetCommand(None))
assertEqual("reset spark.test.property", ResetCommand(Some("spark.test.property")))
Copy link
Contributor

@cloud-fan cloud-fan Jun 14, 2017

Choose a reason for hiding this comment

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

can we add a test for

reset `#a!`

Copy link
Author

@ericsahit ericsahit Jun 15, 2017

Choose a reason for hiding this comment

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

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

assertEqual("reset #$a!", ResetCommand(Some("#$a!")))
Copy link
Contributor

@cloud-fan cloud-fan Jun 16, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

@ericsahit ericsahit Jun 16, 2017

Choose a reason for hiding this comment

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

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)>

}
}
6 changes: 3 additions & 3 deletions sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
}

Seq("reset", s"reset ${SQLConf.GROUP_BY_ORDINAL.key}").foreach { resetCmd =>
test("reset - public conf") {
test(s"reset - public conf $resetCmd") {
Copy link
Contributor

@cloud-fan cloud-fan Jun 26, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

@ericsahit ericsahit Jun 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

done, sounds like a more reasonable name

spark.sessionState.conf.clear()
val original = spark.conf.get(SQLConf.GROUP_BY_ORDINAL)
try {
Expand All @@ -133,7 +133,7 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
}
Copy link
Member

@gatorsmile gatorsmile Jun 13, 2017

Choose a reason for hiding this comment

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

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)
      ...
  }

Copy link
Author

@ericsahit ericsahit Jun 13, 2017

Choose a reason for hiding this comment

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

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


Seq("reset", s"reset ${SQLConf.OPTIMIZER_MAX_ITERATIONS.key}").foreach { resetCmd =>
test("reset - internal conf") {
test(s"reset - internal conf $resetCmd") {
spark.sessionState.conf.clear()
val original = spark.conf.get(SQLConf.OPTIMIZER_MAX_ITERATIONS)
try {
Expand All @@ -151,7 +151,7 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
}

Seq("reset", s"reset $testKey").foreach { resetCmd =>
test("reset - user-defined conf") {
test(s"reset - user-defined conf $resetCmd") {
spark.sessionState.conf.clear()
try {
assert(spark.conf.getOption(testKey).isEmpty)
Expand Down