[SPARK-58640][SQL] Align getBool with the sibling option accessors in CSVOptions and XmlOptions - #57847
Closed
uros-b wants to merge 1 commit into
Closed
[SPARK-58640][SQL] Align getBool with the sibling option accessors in CSVOptions and XmlOptions#57847uros-b wants to merge 1 commit into
uros-b wants to merge 1 commit into
Conversation
uros-b
commented
Aug 7, 2026
uros-b
left a comment
Member
Author
There was a problem hiding this comment.
@cloud-fan Please review.
cloud-fan
approved these changes
Aug 7, 2026
cloud-fan
left a comment
Contributor
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
The refactor is behavior-preserving and internally consistent; no review findings were identified.
Verification
I traced every prior branch against the replacement in both files. An absent key still yields the caller-provided default, an explicit null still yields that default, case-insensitive true and false still map to their Boolean values, and every other non-null string still throws paramIsNotBooleanValueError for the same parameter. No tests were run as part of this review.
Member
Author
|
Thank you @cloud-fan! |
uros-b
added a commit
that referenced
this pull request
Aug 7, 2026
… CSVOptions and XmlOptions
### What changes were proposed in this pull request?
Rewrites the private `getBool` in `CSVOptions` and `XmlOptions` to match the shape of the sibling option accessors:
```scala
private def getBool(paramName: String, default: Boolean = false): Boolean = {
val paramValue = parameters.get(paramName)
paramValue match {
case None => default
case Some(null) => default
case Some(value) => value.toLowerCase(Locale.ROOT) match {
case "true" => true
case "false" => false
case _ => throw QueryExecutionErrors.paramIsNotBooleanValueError(paramName)
}
}
}
```
### Why are the changes needed?
`getChar` and `getInt` in `CSVOptions` already match on `parameters.get(paramName)` and handle the missing and null cases as `case None` and `case Some(null)`, while `getBool` was an if/else-if chain over `parameters.getOrElse(paramName, default.toString)`. Besides being inconsistent, that form converts the default to a string only to parse it straight back, and calls `toLowerCase` twice on the `false` and error paths.
`XmlOptions` carried a byte-identical copy of the old method, so both are updated together to keep them in sync rather than leaving them divergent.
### Does this PR introduce _any_ user-facing change?
No. Behavior is unchanged for every input: an absent key yields the default, an explicit `null` value yields the default, `true`/`false` are accepted case-insensitively via `Locale.ROOT`, and any other value throws the same error with the same parameter as before.
### How was this patch tested?
Behavior-preserving change covered by the existing CSV and XML option tests. Equivalence with the previous implementation was checked case by case across absent keys, explicit nulls, mixed-case `true`/`false`, the empty string, and other invalid values, for both `default` values.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)
Closes #57847 from uros-b/refactor-csvoptions-getbool.
Authored-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
(cherry picked from commit 551a783)
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
uros-b
added a commit
that referenced
this pull request
Aug 7, 2026
… CSVOptions and XmlOptions
### What changes were proposed in this pull request?
Rewrites the private `getBool` in `CSVOptions` and `XmlOptions` to match the shape of the sibling option accessors:
```scala
private def getBool(paramName: String, default: Boolean = false): Boolean = {
val paramValue = parameters.get(paramName)
paramValue match {
case None => default
case Some(null) => default
case Some(value) => value.toLowerCase(Locale.ROOT) match {
case "true" => true
case "false" => false
case _ => throw QueryExecutionErrors.paramIsNotBooleanValueError(paramName)
}
}
}
```
### Why are the changes needed?
`getChar` and `getInt` in `CSVOptions` already match on `parameters.get(paramName)` and handle the missing and null cases as `case None` and `case Some(null)`, while `getBool` was an if/else-if chain over `parameters.getOrElse(paramName, default.toString)`. Besides being inconsistent, that form converts the default to a string only to parse it straight back, and calls `toLowerCase` twice on the `false` and error paths.
`XmlOptions` carried a byte-identical copy of the old method, so both are updated together to keep them in sync rather than leaving them divergent.
### Does this PR introduce _any_ user-facing change?
No. Behavior is unchanged for every input: an absent key yields the default, an explicit `null` value yields the default, `true`/`false` are accepted case-insensitively via `Locale.ROOT`, and any other value throws the same error with the same parameter as before.
### How was this patch tested?
Behavior-preserving change covered by the existing CSV and XML option tests. Equivalence with the previous implementation was checked case by case across absent keys, explicit nulls, mixed-case `true`/`false`, the empty string, and other invalid values, for both `default` values.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)
Closes #57847 from uros-b/refactor-csvoptions-getbool.
Authored-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
(cherry picked from commit 551a783)
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Rewrites the private
getBoolinCSVOptionsandXmlOptionsto match the shape of the sibling option accessors:Why are the changes needed?
getCharandgetIntinCSVOptionsalready match onparameters.get(paramName)and handle the missing and null cases ascase Noneandcase Some(null), whilegetBoolwas an if/else-if chain overparameters.getOrElse(paramName, default.toString). Besides being inconsistent, that form converts the default to a string only to parse it straight back, and callstoLowerCasetwice on thefalseand error paths.XmlOptionscarried a byte-identical copy of the old method, so both are updated together to keep them in sync rather than leaving them divergent.Does this PR introduce any user-facing change?
No. Behavior is unchanged for every input: an absent key yields the default, an explicit
nullvalue yields the default,true/falseare accepted case-insensitively viaLocale.ROOT, and any other value throws the same error with the same parameter as before.How was this patch tested?
Behavior-preserving change covered by the existing CSV and XML option tests. Equivalence with the previous implementation was checked case by case across absent keys, explicit nulls, mixed-case
true/false, the empty string, and other invalid values, for bothdefaultvalues.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)