[SPARK-49775][SQL][TESTS] Make tests of INVALID_PARAMETER_VALUE.CHARSET deterministic
#48235
+17
−17
Conversation
This file contains 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
HyukjinKwon
approved these changes
Sep 25, 2024
zhengruifeng
force-pushed
the
sql_test_sort_charset
branch
from
b956a6c to
348dc51
Compare
HyukjinKwon
approved these changes
Sep 25, 2024
|
Merged to master. |
yaooqinn
reviewed
Sep 25, 2024
| @@ -24,7 +24,7 @@ | |||
| private[sql] object CharsetProvider { | |||
|
|
|||
| final lazy val VALID_CHARSETS = | |||
| Set("us-ascii", "iso-8859-1", "utf-8", "utf-16be", "utf-16le", "utf-16", "utf-32") | |||
| Array("us-ascii", "iso-8859-1", "utf-8", "utf-16be", "utf-16le", "utf-16", "utf-32").sorted | |||
There was a problem hiding this comment.
Is it more performant to use the contains of set than array?
There was a problem hiding this comment.
It's happening once anyway so shouldn't be a biggie. invalidCharsetError happens for every call
There was a problem hiding this comment.
charset can be a column value，this could be expensive？
There was a problem hiding this comment.
Hm, in that case, we should probably keep this as a set, and just create another variable sorted.
There was a problem hiding this comment.
https://www.scala-lang.org/api/2.12.5/scala/collection/SortedSet$.html maybe we can use a SortedSet here
dongjoon-hyun
pushed a commit
that referenced
this pull request
Sep 25, 2024
…ting ### What changes were proposed in this pull request? This PR is a followup of #48235 that addresses #48235 (comment) comment. ### Why are the changes needed? For better performance (in theory) ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests should verify them ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48245 from HyukjinKwon/SPARK-49775-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
attilapiros
pushed a commit
to attilapiros/spark
that referenced
this pull request
Oct 4, 2024
…SET` deterministic
### What changes were proposed in this pull request?
Make tests of `INVALID_PARAMETER_VALUE.CHARSET` deterministic
### Why are the changes needed?
`VALID_CHARSETS` is a Set, so `VALID_CHARSETS.mkString(", ")` is non-deterministic, and cause failures in different testing environments, e.g.
```
org.scalatest.exceptions.TestFailedException: ansi/string-functions.sql
Expected "...sets" : "UTF-16LE, U[TF-8, UTF-32, UTF-16BE, UTF-16, US-ASCII, ISO-8859-1]",
"functionName...", but got "...sets" : "UTF-16LE, U[S-ASCII, ISO-8859-1, UTF-8, UTF-32, UTF-16BE, UTF-16]",
"functionName..." Result did not match for query apache#93
select encode('hello', 'WINDOWS-1252')
at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
at org.scalatest.funsuite.AnyFunSuite.newAssertionFailedException(AnyFunSuite.scala:1564)
at org.scalatest.Assertions.assertResult(Assertions.scala:847)
at org.scalatest.Assertions.assertResult$(Assertions.scala:842)
at org.scalatest.funsuite.AnyFunSuite.assertResult(AnyFunSuite.scala:1564)
```
### Does this PR introduce _any_ user-facing change?
No, test only
### How was this patch tested?
updated tests
### Was this patch authored or co-authored using generative AI tooling?
no
Closes apache#48235 from zhengruifeng/sql_test_sort_charset.
Authored-by: Ruifeng Zheng <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
attilapiros
pushed a commit
to attilapiros/spark
that referenced
this pull request
Oct 4, 2024
…ting ### What changes were proposed in this pull request? This PR is a followup of apache#48235 that addresses apache#48235 (comment) comment. ### Why are the changes needed? For better performance (in theory) ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests should verify them ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48245 from HyukjinKwon/SPARK-49775-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
himadripal
pushed a commit
to himadripal/spark
that referenced
this pull request
Oct 19, 2024
…SET` deterministic
### What changes were proposed in this pull request?
Make tests of `INVALID_PARAMETER_VALUE.CHARSET` deterministic
### Why are the changes needed?
`VALID_CHARSETS` is a Set, so `VALID_CHARSETS.mkString(", ")` is non-deterministic, and cause failures in different testing environments, e.g.
```
org.scalatest.exceptions.TestFailedException: ansi/string-functions.sql
Expected "...sets" : "UTF-16LE, U[TF-8, UTF-32, UTF-16BE, UTF-16, US-ASCII, ISO-8859-1]",
"functionName...", but got "...sets" : "UTF-16LE, U[S-ASCII, ISO-8859-1, UTF-8, UTF-32, UTF-16BE, UTF-16]",
"functionName..." Result did not match for query apache#93
select encode('hello', 'WINDOWS-1252')
at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
at org.scalatest.funsuite.AnyFunSuite.newAssertionFailedException(AnyFunSuite.scala:1564)
at org.scalatest.Assertions.assertResult(Assertions.scala:847)
at org.scalatest.Assertions.assertResult$(Assertions.scala:842)
at org.scalatest.funsuite.AnyFunSuite.assertResult(AnyFunSuite.scala:1564)
```
### Does this PR introduce _any_ user-facing change?
No, test only
### How was this patch tested?
updated tests
### Was this patch authored or co-authored using generative AI tooling?
no
Closes apache#48235 from zhengruifeng/sql_test_sort_charset.
Authored-by: Ruifeng Zheng <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
himadripal
pushed a commit
to himadripal/spark
that referenced
this pull request
Oct 19, 2024
…ting ### What changes were proposed in this pull request? This PR is a followup of apache#48235 that addresses apache#48235 (comment) comment. ### Why are the changes needed? For better performance (in theory) ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests should verify them ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48245 from HyukjinKwon/SPARK-49775-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
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?
Make tests of
INVALID_PARAMETER_VALUE.CHARSETdeterministicWhy are the changes needed?
VALID_CHARSETSis a Set, soVALID_CHARSETS.mkString(", ")is non-deterministic, and cause failures in different testing environments, e.g.Does this PR introduce any user-facing change?
No, test only
How was this patch tested?
updated tests
Was this patch authored or co-authored using generative AI tooling?
no