You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "xi-db (via GitHub)" <gi...@apache.org> on 2024/09/26 13:35:40 UTC
[PR] [SPARK-47341] Fix inaccurate documentation of RuntimeConfig.get [spark]
xi-db opened a new pull request, #48264:
URL: https://github.com/apache/spark/pull/48264
### What changes were proposed in this pull request?
The existing documentation of `RuntimeConfig.get()` is misleading:
* `get(key: String)` method will not throw any exception if the key is not set as long as the config entry has a default value, instead, they will just return the `defaultValue` of the `ConfigEntry`. An `NoSuchElementException` will only be thrown if there is no default value for the config entry.
* `get(key: String, default: String)` method will ignore the `defaultValue` of its `ConfigEntry`, and return the given param `default` if unset.
* `getOption(key: String)` method will return the `defaultValue` of its `ConfigEntry` if the config is not set.
An example:
This line makes sure the config is not set.
```
scala> spark.conf.unset("spark.sql.session.timeZone")
```
The following code returns `Etc/UTC`, which doesn't throw any exception.
```
scala> spark.conf.get("spark.sql.session.timeZone")
res2: String = "Etc/UTC"
```
The following code returns `Europe/Berlin`, ignoring the default value. However, the documentation only says it returns the value, without mentioning ignoring the default value of the entry when the config is not explicitly set.
```
scala> spark.conf.get("spark.sql.session.timeZone", "Europe/Berlin")
res3: String = "Europe/Berlin"
```
In this PR, the documentation is fixed and a new test case is added.
### Why are the changes needed?
The incorrect documentation is likely to mislead users to weird behaviours if they rely on the documentation.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New test case in `RuntimeConfigSuite`.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47341][SQL] Fix inaccurate documentation of RuntimeConfig#get [spark]
Posted by "xi-db (via GitHub)" <gi...@apache.org>.
xi-db commented on PR #48264:
URL: https://github.com/apache/spark/pull/48264#issuecomment-2379643067
Hi @HyukjinKwon , I reformated the code, but the `Documentation Generation` still fails. Do you have any ideas how I can fix it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47341][SQL] Fix inaccurate documentation of RuntimeConfig.get [spark]
Posted by "xi-db (via GitHub)" <gi...@apache.org>.
xi-db commented on PR #48264:
URL: https://github.com/apache/spark/pull/48264#issuecomment-2377132089
@HyukjinKwon Could you take a look at this documentation improvement on `RuntimeConfig#get`? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47341][SQL] Fix inaccurate documentation of RuntimeConfig#get [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #48264:
URL: https://github.com/apache/spark/pull/48264#discussion_r1779283866
##########
sql/api/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala:
##########
@@ -54,17 +54,20 @@ abstract class RuntimeConfig {
}
/**
- * Returns the value of Spark runtime configuration property for the given key.
+ * Returns the value of Spark runtime configuration property for the given key. If the key is
+ * not set yet, return `defaultValue` in its [[ConfigEntry]].
Review Comment:
I think real error is this:
```
```
[error] Generating /__w/spark/spark/target/javaunidoc/org/apache/spark/sql/RuntimeConfig.html...
[error] /__w/spark/spark/sql/api/target/java/org/apache/spark/sql/RuntimeConfig.java:13:1: error: reference not found
[error] * not set yet, return <code>defaultValue</code> in its {@link ConfigEntry}.
[error] ^
[error] /__w/spark/spark/sql/api/target/java/org/apache/spark/sql/RuntimeConfig.java:25:1: error: reference not found
[error] * {@link ConfigEntry} is not the desired one.
[error] ^
```
```
Let's just switch `[[ConfigEntry]]` to something else or remove it. `ConfigEntry` isn't technically an API so there is no way for the end users to see.
##########
sql/api/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala:
##########
@@ -54,17 +54,20 @@ abstract class RuntimeConfig {
}
/**
- * Returns the value of Spark runtime configuration property for the given key.
+ * Returns the value of Spark runtime configuration property for the given key. If the key is
+ * not set yet, return `defaultValue` in its [[ConfigEntry]].
Review Comment:
I think real error is this:
```
[error] Generating /__w/spark/spark/target/javaunidoc/org/apache/spark/sql/RuntimeConfig.html...
[error] /__w/spark/spark/sql/api/target/java/org/apache/spark/sql/RuntimeConfig.java:13:1: error: reference not found
[error] * not set yet, return <code>defaultValue</code> in its {@link ConfigEntry}.
[error] ^
[error] /__w/spark/spark/sql/api/target/java/org/apache/spark/sql/RuntimeConfig.java:25:1: error: reference not found
[error] * {@link ConfigEntry} is not the desired one.
[error] ^
```
Let's just switch `[[ConfigEntry]]` to something else or remove it. `ConfigEntry` isn't technically an API so there is no way for the end users to see.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47341][SQL] Fix inaccurate documentation of RuntimeConfig#get [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #48264:
URL: https://github.com/apache/spark/pull/48264#issuecomment-2378130475
Let's make CI happy and green though :-).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org