Skip to content
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

Jump to bottom

[MINOR][SQL] Add missing functions for some options in SQLConf and use them where applicable #14678

Closed
wants to merge 4 commits into from
Closed

[MINOR][SQL] Add missing functions for some options in SQLConf and use them where applicable #14678

wants to merge 4 commits into from

Conversation

HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Aug 17, 2016 •
edited

What changes were proposed in this pull request?

I first thought they are missing because they are kind of hidden options but it seems they are just missing.

For example, spark.sql.parquet.mergeSchema is documented in sql-programming-guide.md but this function is missing whereas many options such as spark.sql.join.preferSortMergeJoin are not documented but have its own function individually.

So, this PR suggests making them consistent by adding the missing functions for some options in SQLConf and use them where applicable, in order to make them more readable.

How was this patch tested?

Existing tests should cover this.

@SparkQA
Copy link

SparkQA commented Aug 17, 2016

Test build #63898 has finished for PR 14678 at commit c959f3b.

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

HyukjinKwon
HyukjinKwon reviewed Aug 17, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
@@ -322,11 +322,6 @@ object SQLConf {
.intConf
.createWithDefault(4000)

val PARTITION_DISCOVERY_ENABLED = SQLConfigBuilder("spark.sql.sources.partitionDiscovery.enabled")
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems we always enable this and this option is not referenced anywhere(?). Is this intendedly not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems the usage was removed while refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

Could you do me a favor to check which PR removed the usage?

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like we still keep another SQLConf PARALLEL_PARTITION_DISCOVERY_THRESHOLD. However, the description of PARALLEL_PARTITION_DISCOVERY_THRESHOLD is not right after the refactoring. Could you update the descriptions too?

Copy link
Member

Choose a reason for hiding this comment

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

PR #11509 removes the usage of PARTITION_DISCOVERY_ENABLED. I think it is safe to remove this conf now.

@HyukjinKwon
Copy link
Member Author

cc @rxin, Could you check if this make sense please?

@rxin
Copy link
Contributor

rxin commented Aug 17, 2016

Looks good, but I didn't look super carefully.

@gatorsmile do you have time to take a more careful look at this?

@SparkQA
Copy link

SparkQA commented Aug 17, 2016

Test build #63899 has finished for PR 14678 at commit a57dd5e.

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

@gatorsmile
Copy link
Member

Sure, will review it today. Thanks!

@gatorsmile
Copy link
Member

Just searched the code and found more. Any reason why we do not change these?

ParquetFileFormat.scala:85:    if (conf.get(SQLConf.PARQUET_OUTPUT_COMMITTER_CLASS.key) == null) {
ParquetSchemaConverter.scala:66:    assumeBinaryIsString = conf.get(
ParquetSchemaConverter.scala:67:    assumeInt96IsTimestamp = conf.get(
ParquetSchemaConverter.scala:68:    writeLegacyParquetFormat = conf.get(
ParquetWriteSupport.scala:80:      assert(configuration.get(SQLConf.PARQUET_WRITE_LEGACY_FORMAT.key) != 
ParquetWriteSupport.scala:81:      configuration.get(SQLConf.PARQUET_WRITE_LEGACY_FORMAT.key).toBoolean

@gatorsmile
Copy link
Member

nvm, I got the answer. We are unable to change them. Thanks!

@gatorsmile
Copy link
Member

I checked the conf changes and the data types. It looks good to me. Only leave a minor comment on the conf description of one related parameter PARALLEL_PARTITION_DISCOVERY_THRESHOLD. Thanks!

@HyukjinKwon
Copy link
Member Author

@gatorsmile Thanks for a very close look. I should've check the description of PARALLEL_PARTITION_DISCOVERY_THRESHOLD.

@gatorsmile
Copy link
Member

LGTM pending tests

@SparkQA
Copy link

SparkQA commented Aug 18, 2016

Test build #63959 has finished for PR 14678 at commit a26c08e.

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

@HyukjinKwon
Copy link
Member Author

(@rxin cc you in case you just missed this)

@HyukjinKwon
Copy link
Member Author

ping @rxin

@HyukjinKwon
Copy link
Member Author

@cloud-fan Do you mind if I ask to take a look please?

@cloud-fan
Copy link
Contributor

retest this please

@cloud-fan
Copy link
Contributor

LGTM

@SparkQA
Copy link

SparkQA commented Sep 12, 2016

Test build #65263 has finished for PR 14678 at commit a26c08e.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@asfgit asfgit closed this in a79838b Sep 14, 2016
@rxin
Copy link
Contributor

rxin commented Sep 16, 2016

Thanks but also FYI this is not "minor". We should have a JIRA ticket for changes like this in the future.

wgtmac pushed a commit to wgtmac/spark that referenced this pull request Sep 19, 2016
@HyukjinKwon @wgtmac
[MINOR][SQL] Add missing functions for some options in SQLConf and us…
08b9c75 
…e them where applicable

## What changes were proposed in this pull request?

I first thought they are missing because they are kind of hidden options but it seems they are just missing.

For example, `spark.sql.parquet.mergeSchema` is documented in [sql-programming-guide.md](https://github.com/apache/spark/blob/master/docs/sql-programming-guide.md) but this function is missing whereas many options such as `spark.sql.join.preferSortMergeJoin` are not documented but have its own function individually.

So, this PR suggests making them consistent by adding the missing functions for some options in `SQLConf` and use them where applicable, in order to make them more readable.

## How was this patch tested?

Existing tests should cover this.

Author: hyukjinkwon <gurwls223@gmail.com>

Closes apache#14678 from HyukjinKwon/sqlconf-cleanup.
@zsxwing zsxwing mentioned this pull request Oct 12, 2016
Closed
@HyukjinKwon HyukjinKwon deleted the sqlconf-cleanup branch January 2, 2018 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@HyukjinKwon @SparkQA @rxin @gatorsmile @cloud-fan