You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by kevinyu98 <gi...@git.apache.org> on 2015/12/15 20:35:11 UTC
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
GitHub user kevinyu98 opened a pull request:
https://github.com/apache/spark/pull/10314
[SPARK-12317][SQL]Support configurable value in SQLConf file
Hello: adding the configure value for AUTO_BROADCASTJOIN_THRESHOLD and DEFAULT_SIZE_IN_BYTES.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kevinyu98/spark working_on_spark-12317
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/10314.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #10314
----
commit 44e7fba419088a589cbeeb6cbf012f43ad49576c
Author: Kevin Yu <qy...@us.ibm.com>
Date: 2015-12-15T19:25:28Z
fix spark jira 12317
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurate value fo...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-169173265
Hello @srowen @marmbrus @viirya : I have made the code changes, and change the title based on the comments. Can you help review the codes? Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48833641
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,37 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ val isInteger: Option[Int] = try {
--- End diff --
I find this body simpler:
```
try {
v.toInt
} catch {
case _: NumberFormatException =>
val sizeInBytes = try {
Utils.byteStringAsBytes(v)
} catch {
case _: NumberFormatException =>
throw new IllegalArgumentException(s"$key should be int, but was $v")
}
require(getSizeAsBytes <= Int.MaxValue && getSizeAsBytes >= Int.MinValue)
sizeInBytes.toInt
}
```
I'd like to hear from @marmbrus or @viirya for example before merging a change like this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support units (m,k,g) in SQL...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-169438242
Hello @marmbrus, thanks. So your mean I can remove the code change for intMemConf, and keep the code for longMemConf for this jira? I will make the PR title and description changes. I need to close this PR, open another one, seems there is some issues when I did last git push.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r47713626
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -93,7 +94,7 @@ private[spark] object SQLConf {
isPublic: Boolean = true): SQLConfEntry[Int] =
SQLConfEntry(key, defaultValue, { v =>
try {
- v.toInt
+ Utils.byteStringAsBytes(v).toInt
--- End diff --
Hello Sean: Thanks for your comment. Yes, you are right. There are other methods are not meaning to use memory sizes. (like COLUMN_BATCH_SIZE, etc). There are couple approaches, can you suggest which way is preferable way for this problem or suggest a new way to fix this ?
1. we will document that [g|G|m|M|k|K] means memory size.
2. create a new method of intConf for AUTO_BROADCASTJOIN_THRESHOLD.
3. create a rule to the parseByteString. like K/KB/M/MB means 1024, k/kb/m/mb means 1000.
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48334198
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ var isNegative: Boolean = false
+ try {
+ isNegative = (v.toInt < 0)
+ } catch {
+ case _: Throwable =>
+ }
+ if (!isNegative) {
--- End diff --
It's clearer to flip this conditional.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-165010421
@watermen thanks for your input. It is good idea if we decide to go with approach 2. create a new method of intConf for AUTO_BROADCASTJOIN_THRESHOLD.
If we decide to go with approach 3, then we may need to change the parseByteString part to distinct lower and upper case.
@srowen what do you think? Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48334129
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
--- End diff --
The spacing and alignment are wrong throughout this method
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48451915
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ var isNegative: Boolean = false
+ try {
+ isNegative = (v.toInt < 0)
+ } catch {
+ case _: Throwable =>
--- End diff --
yah, I want to catch the exception, then do nothing. I will make the changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48334144
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ var isNegative: Boolean = false
--- End diff --
Why `var`? assign the result of the try to a `val`. No need for a type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-166560467
@srowen Hello Sean: I am sorry that I have't been able to submit the PR yet. I will conti. work on it tomorrow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-168316840
@kevinyu98 are you still working on this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-166747545
@srowen Hello Sean: I have submit the new code, can you help review? Thanks a lot.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48451909
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
--- End diff --
I will make the changes. I used the code format and run the scalastyle test, thought it passed the styles. I will look more carefully next time. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-166855061
Are there other configs that should take a memory string?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r47692476
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -93,7 +94,7 @@ private[spark] object SQLConf {
isPublic: Boolean = true): SQLConfEntry[Int] =
SQLConfEntry(key, defaultValue, { v =>
try {
- v.toInt
+ Utils.byteStringAsBytes(v).toInt
--- End diff --
It looks like you're making all integer config values parse as if they're memory sizes. That's not right, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48334101
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -107,7 +135,7 @@ private[spark] object SQLConf {
isPublic: Boolean = true): SQLConfEntry[Long] =
SQLConfEntry(key, defaultValue, { v =>
try {
- v.toLong
+ Utils.byteStringAsBytes(v)
--- End diff --
This is still wrong
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-169086076
This seems like a nice convenience for the users. A few comments:
- The description / title could be improved. Some think about support units for configuration.
- Do we really need both an Int and a Long version? (maybe we do)
- Sean's suggestions seems good, but I would include a better error than the default for the `require`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-165022104
It does seem like you would want a separate utility method for handling configuration values that are intended to be memory sizes.
Don't make up new behavior; just use the behavior from the `Utils` method. In particular, technically, "k" means 1000 and "Ki" means 1024, but I think we follow the JVM memory string conventions instead.
I don't see why `toString.toInt` solves an overflow problem. You get a `long` back, and if one particular value must be an `int`, simply handle argument checking for that value directly. No need for new methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by watermen <gi...@git.apache.org>.
Github user watermen commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-164978133
@kevinyu98 I think we can add a new method like `intConfWithMem`, and also need to check the value in bytes need to LessThanOrEqual `Int.MaxValue`.
`K/KB/M/MB` is equals to `k/kb/m/mb`, right? See the code from JavaUtils below.
```
private static long parseByteString(String str, ByteUnit unit) {
String lower = str.toLowerCase().trim();
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurate value fo...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-169220626
Hello @viirya : Good Point. I just test it, -1 and -1g will have different behavior. It will take -1, and throw IllegalArgumentException for -1g. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurate value fo...
Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48992838
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -214,7 +256,7 @@ private[spark] object SQLConf {
doc = "When true, enable partition pruning for in-memory columnar tables.",
isPublic = false)
- val AUTO_BROADCASTJOIN_THRESHOLD = intConf("spark.sql.autoBroadcastJoinThreshold",
+ val AUTO_BROADCASTJOIN_THRESHOLD = intMemConf("spark.sql.autoBroadcastJoinThreshold",
--- End diff --
How about we just use `longMemConf`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48334213
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ var isNegative: Boolean = false
+ try {
+ isNegative = (v.toInt < 0)
+ } catch {
+ case _: Throwable =>
+ }
+ if (!isNegative) {
+ if ((Utils.byteStringAsBytes(v) <= Int.MaxValue.toLong) &&
--- End diff --
Why compute this 3 times?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurate value fo...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-169214001
Besides, -1 and -1g will be the same or different behavior?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48451947
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -107,7 +135,7 @@ private[spark] object SQLConf {
isPublic: Boolean = true): SQLConfEntry[Long] =
SQLConfEntry(key, defaultValue, { v =>
try {
- v.toLong
+ Utils.byteStringAsBytes(v)
--- End diff --
sorry, I thought it was only one place. But actually there are two places. I will create a new method for SHUFFLE_TARGET_POSTSHUFFLE_INPUT_SIZE. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-168317362
Hello Sean: Sorry for the delay. Yes, I have made most code changes, and I will try to finish it up soon and do more testing. Will keep you updated. Thanks, Happy New Year !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48451916
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ var isNegative: Boolean = false
+ try {
+ isNegative = (v.toInt < 0)
+ } catch {
+ case _: Throwable =>
+ }
+ if (!isNegative) {
--- End diff --
ok
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-166279657
@kevinyu98 are you still working on this one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48451911
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ var isNegative: Boolean = false
--- End diff --
ok, I will make the change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48451918
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ var isNegative: Boolean = false
+ try {
+ isNegative = (v.toInt < 0)
+ } catch {
+ case _: Throwable =>
+ }
+ if (!isNegative) {
+ if ((Utils.byteStringAsBytes(v) <= Int.MaxValue.toLong) &&
--- End diff --
I will put it in a variable. thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurate value fo...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-169417153
I think just `long` is fine for now. The title still has spelling errors and is really long. I would say:
`[SPARK-12317][SQL] Support units (m,k,g) in SQLConf`. In the description of the PR (which becomes the rest of the commit message) you can spell out which options you change and more details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support units (m,k,g) in SQL...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 closed the pull request at:
https://github.com/apache/spark/pull/10314
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurate value fo...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-169213838
I have the same question as @marmbrus, do we need two methods for int and long? Looks like they are very similar.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-166326525
@srowen Hello Sean, yes, sorry for the delay. I will submit the updated PR today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-164870635
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10314#discussion_r48334160
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -100,6 +101,33 @@ private[spark] object SQLConf {
}
}, _.toString, doc, isPublic)
+ def intMemConf(
+ key: String,
+ defaultValue: Option[Int] = None,
+ doc: String = "",
+ isPublic: Boolean = true): SQLConfEntry[Int] =
+ SQLConfEntry(key, defaultValue, { v =>
+ var isNegative: Boolean = false
+ try {
+ isNegative = (v.toInt < 0)
+ } catch {
+ case _: Throwable =>
--- End diff --
Why `Throwable`? I assume you mean `NumberFormatException`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurate value fo...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-169238048
@viirya @yhuai @srowen @marmbrus @concretevitamin: SHUFFLE_TARGET_POSTSHUFFLE_INPUT_SIZE was added by spark-9850, PR#9276, I am including Yin ,
Michael set the autoBroadcastJoinThreshold to 10 * 1024 * 1024 through PR3064,
spark-2393 set the autoBroadcastJoinThreshold to Int.
I am not sure which one to choose either, so I cc on the persons who introduce these two fields. Thanks for your input.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-165023639
@srowen Thanks Sean, I will create a new method of intConf for AUTO_BROADCASTJOIN_THRESHOLD. Will update the PR soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-12317][SQL]Support configurable value i...
Posted by kevinyu98 <gi...@git.apache.org>.
Github user kevinyu98 commented on the pull request:
https://github.com/apache/spark/pull/10314#issuecomment-168854235
@srowen Hello Sean: Sorry for taking so long. Can you review the code? Thanks. Kevin
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org