-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-37084][SQL] Set spark.sql.files.openCostInBytes to bytesConf #34353
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.
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
Conversation
| .longConf | ||
| .createWithDefault(4 * 1024 * 1024) | ||
| .bytesConf(ByteUnit.BYTE) | ||
| .createWithDefaultString("4MB") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a simple test at ConfigEntrySuite to make sure byte configuration is able to take the long type numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1; we could probably extend this existing test:
spark/core/src/test/scala/org/apache/spark/internal/config/ConfigEntrySuite.scala
Lines 90 to 97 in 4148fb5
| test("conf entry: bytes") { | |
| val conf = new SparkConf() | |
| val bytes = ConfigBuilder(testKey("bytes")).bytesConf(ByteUnit.KiB) | |
| .createWithDefaultString("1m") | |
| assert(conf.get(bytes) === 1024L) | |
| conf.set(bytes.key, "1k") | |
| assert(conf.get(bytes) === 1L) | |
| } |
|
Let's also file a JIRA, see also https://spark.apache.org/contributing.html |
|
ok to test |
|
Merged to master. |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #144550 has finished for PR 34353 at commit
|
|
Can one of the admins verify this patch? |
### What changes were proposed in this pull request? Set `spark.sql.files.openCostInBytes` to bytesConf. ### Why are the changes needed? The name is _*InBytes_, but it actually only accepts **long type**. This is confusing for users. After the changes, it can accept **String** as input which is more flexible to users. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing test. Closes #34353 from RabbidHY/SPARK-37084. Authored-by: RabbidHY <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
What changes were proposed in this pull request?
Set
spark.sql.files.openCostInBytesto bytesConf.Why are the changes needed?
The name is *InBytes, but it actually only accepts long type. This is confusing for users. After the changes, it can accept String as input which is more flexible to users.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing test.