Skip to content

[SPARK-28257][SQL] Use ConfigEntry for hardcoded configs in SQL - #25059

Closed
WangGuangxin wants to merge 5 commits into
apache:masterfrom
WangGuangxin:ConfigEntry
Closed

[SPARK-28257][SQL] Use ConfigEntry for hardcoded configs in SQL#25059
WangGuangxin wants to merge 5 commits into
apache:masterfrom
WangGuangxin:ConfigEntry

Conversation

@WangGuangxin

@WangGuangxinWangGuangxin commented Jul 5, 2019

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

There are some hardcoded configs, using config entry to replace them.

How was this patch tested?

Existing UT

@WangGuangxinWangGuangxin changed the title [SPARK-28257]Use ConfigEntry for hardcoded configs in SQL[SPARK-28257][SQL][MINOR]Use ConfigEntry for hardcoded configs in SQLJul 5, 2019
wangyum
wangyum reviewed Jul 5, 2019
View reviewed changes

@wangyumwangyum left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you replace more configs:

spark.sql.warehouse.dir
spark.sql.sources.schemaStringLengthThreshold
spark.sql.hive.thriftServer.singleSession
spark.sql.extensions
spark.sql.streaming.streamingQueryListeners
spark.sql.ui.retainedExecutions

@jerryshao

jerryshao commented Jul 5, 2019

Copy link
Copy Markdown
Contributor

ok to test.

@SparkQA

SparkQA commented Jul 5, 2019

Copy link
Copy Markdown

Test build #107283 has finished for PR 25059 at commit 16236b7.

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

@dongjoon-hyun

dongjoon-hyun commented Jul 5, 2019

Copy link
Copy Markdown
Member

Hi, @WangGuangxin . We don't need to put [MINOR] if you have a JIRA ID.

@dongjoon-hyundongjoon-hyun changed the title [SPARK-28257][SQL][MINOR]Use ConfigEntry for hardcoded configs in SQL[SPARK-28257][SQL] Use ConfigEntry for hardcoded configs in SQLJul 5, 2019
@WangGuangxin

WangGuangxin commented Jul 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Could you replace more configs:

spark.sql.warehouse.dir
spark.sql.sources.schemaStringLengthThreshold
spark.sql.hive.thriftServer.singleSession
spark.sql.extensions
spark.sql.streaming.streamingQueryListeners
spark.sql.ui.retainedExecutions

done

@SparkQA

SparkQA commented Jul 6, 2019

Copy link
Copy Markdown

Test build #107296 has finished for PR 25059 at commit 9198d28.

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

@dongjoon-hyun

dongjoon-hyun commented Jul 10, 2019

Copy link
Copy Markdown
Member

Retest this please.

@SparkQA

SparkQA commented Jul 10, 2019

Copy link
Copy Markdown

Test build #107476 has finished for PR 25059 at commit 9198d28.

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

dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/sources/CreateTableAsSelectSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/sources/CreateTableAsSelectSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/hive/src/test/scala/org/apache/spark/sql/hive/test/TestHive.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/hive/src/test/scala/org/apache/spark/sql/hive/test/TestHive.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/hive/src/test/scala/org/apache/spark/sql/hive/test/TestHive.scala Outdated
@dongjoon-hyun

dongjoon-hyun commented Jul 10, 2019

Copy link
Copy Markdown
Member

This is a little huge for this kind of patch. However, it looks correct. I finished my first round review. Could you update this PR, @WangGuangxin ?

maropu
maropu reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated
maropu
maropu reviewed Jul 10, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated
@SparkQA

SparkQA commented Jul 11, 2019

Copy link
Copy Markdown

Test build #107533 has finished for PR 25059 at commit 89da7a9.

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

@WangGuangxin

WangGuangxin commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

This is a little huge for this kind of patch. However, it looks correct. I finished my first round review. Could you update this PR, @WangGuangxin ?

Thanks for your comments. I believe I've resolved all related comments & conflicts.

@SparkQA

SparkQA commented Jul 11, 2019

Copy link
Copy Markdown

Test build #107541 has finished for PR 25059 at commit e601c74.

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

dongjoon-hyun
dongjoon-hyun reviewed Jul 11, 2019
View reviewed changes
Comment threadsql/hive/src/test/scala/org/apache/spark/sql/hive/test/TestHive.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 11, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/RuntimeConfigSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 11, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/RuntimeConfigSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 11, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/execution/SQLWindowFunctionSuite.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 11, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/execution/SQLWindowFunctionSuite.scala Outdated
@dongjoon-hyun

dongjoon-hyun commented Jul 11, 2019

Copy link
Copy Markdown
Member

Thank you for updating, @WangGuangxin . There are a few more remaining nits.

@SparkQA

SparkQA commented Jul 12, 2019

Copy link
Copy Markdown

Test build #107568 has finished for PR 25059 at commit b39fb62.

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

dongjoon-hyun
dongjoon-hyun approved these changes Jul 12, 2019
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, LGTM. Merged to master.
Thank you, @WangGuangxin and @maropu !

@WangGuangxin
WangGuangxin deleted the ConfigEntry branch July 12, 2019 09:32
vinodkc pushed a commit to vinodkc/spark that referenced this pull request Jul 18, 2019
@WangGuangxin@vinodkc
[SPARK-28257][SQL] Use ConfigEntry for hardcoded configs in SQL
6cc4b8b
## What changes were proposed in this pull request?
There are some hardcoded configs, using config entry to replace them.
## How was this patch tested?
Existing UT
Closesapache#25059 from WangGuangxin/ConfigEntry.
Authored-by: wangguangxin.cn <wangguangxin.cn@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropumaropumaropu left review comments
@wangyumwangyumwangyum left review comments
@dongjoon-hyundongjoon-hyundongjoon-hyun approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@WangGuangxin@jerryshao@SparkQA@dongjoon-hyun@maropu@wangyum