You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "ueshin (via GitHub)" <gi...@apache.org> on 2023/02/23 23:09:35 UTC
[GitHub] [spark] ueshin opened a new pull request, #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
ueshin opened a new pull request, #40150:
URL: https://github.com/apache/spark/pull/40150
### What changes were proposed in this pull request?
Implements `SparkSession.conf`.
Took #39995 over.
### Why are the changes needed?
`SparkSession.conf` is a missing feature.
### Does this PR introduce _any_ user-facing change?
Yes, `SparkSession.conf` will be available.
### How was this patch tested?
Added/enabled related tests.
--
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
[GitHub] [spark] HyukjinKwon commented on pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #40150:
URL: https://github.com/apache/spark/pull/40150#issuecomment-1444877430
Merged to master and branch-3.4.
--
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
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40150:
URL: https://github.com/apache/spark/pull/40150#discussion_r1116654223
##########
python/pyspark/sql/conf.py:
##########
@@ -35,12 +35,14 @@ def __init__(self, jconf: JavaObject) -> None:
self._jconf = jconf
@since(2.0)
- def set(self, key: str, value: str) -> None:
+ def set(self, key: str, value: Union[str, int, bool]) -> None:
"""Sets the given Spark runtime configuration property."""
Review Comment:
If this gets merged soon, I think we can just mark it as 3.4.0.
--
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
[GitHub] [spark] ueshin commented on a diff in pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
Posted by "ueshin (via GitHub)" <gi...@apache.org>.
ueshin commented on code in PR #40150:
URL: https://github.com/apache/spark/pull/40150#discussion_r1117302257
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,141 @@ message ExecutePlanResponse {
}
}
+// The placeholder for the config request and response when the values can be optional.
+message OptionalValue {
+ optional string value = 1;
+}
+
+// The key-value pair for the config request and response.
+message KeyValue {
+ string key = 1;
+ string value = 2;
+}
+
+// The key-value pair for the config request and response when the value can be optional.
+message OptionalKeyValue {
+ string key = 1;
+ OptionalValue value = 2;
+}
+
+// Request to update or fetch the configurations.
+message ConfigRequest {
+ // (Required)
+ //
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // (Required) User context
+ UserContext user_context = 2;
+
+ // (Required) The operation for the config.
+ Operation operation = 3;
+
+ // Provides optional information about the client sending the request. This field
+ // can be used for language or version specific information and is only intended for
+ // logging purposes and will not be interpreted by the server.
+ optional string client_type = 4;
+
+ message Operation {
+ oneof op_type {
+ Set set = 1;
+ Get get = 2;
+ GetWithDefault get_with_default = 3;
+ GetOption get_option = 4;
+ GetAll get_all = 5;
+ Unset unset = 6;
+ IsModifiable is_modifiable = 7;
+ }
+ }
+
+ message Set {
+ // (Required) The config key-value pairs to set.
+ repeated KeyValue pairs = 1;
+ }
+
+ message Get {
+ // (Required) The config keys to get.
+ repeated string keys = 1;
+ }
+
+ message GetWithDefault {
+ // (Required) The config key-value paris to get. The value will be used as the default value.
+ repeated OptionalKeyValue pairs = 1;
+ }
+
+ message GetOption {
+ // (Required) The config keys to get optionally.
+ repeated string keys = 1;
+ }
+
+ message GetAll {
+ // (Optional) The prefix of the config key to get.
+ optional string prefix = 1;
+ }
+
+ message Unset {
+ // (Required) The config keys to unset.
+ repeated string keys = 1;
+ }
+
+ message IsModifiable {
+ // (Required) The config keys to check the config is modifiable.
+ repeated string keys = 1;
+ }
+}
+
+// Response to the config request.
+message ConfigResponse {
+ string client_id = 1;
+
+ // (Required) The operation for the config.
+ Operation operation = 2;
+
+ // (Optional)
+ //
+ // Warning messages for deprecated or unsupported configurations.
+ repeated string warnings = 3;
+
+ message Operation {
+ oneof op_type {
+ Set set = 1;
+ Get get = 2;
+ GetWithDefault get_with_default = 3;
+ GetOption get_option = 4;
+ GetAll get_all = 5;
+ Unset unset = 6;
+ IsModifiable is_modifiable = 7;
+ }
+ }
Review Comment:
What about the response for is_modifiable? The value should not be string.
--
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
[GitHub] [spark] HyukjinKwon closed pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
URL: https://github.com/apache/spark/pull/40150
--
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
[GitHub] [spark] grundprinzip commented on a diff in pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #40150:
URL: https://github.com/apache/spark/pull/40150#discussion_r1116933885
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,141 @@ message ExecutePlanResponse {
}
}
+// The placeholder for the config request and response when the values can be optional.
+message OptionalValue {
+ optional string value = 1;
+}
+
+// The key-value pair for the config request and response.
+message KeyValue {
+ string key = 1;
+ string value = 2;
+}
+
+// The key-value pair for the config request and response when the value can be optional.
+message OptionalKeyValue {
+ string key = 1;
+ OptionalValue value = 2;
+}
+
+// Request to update or fetch the configurations.
+message ConfigRequest {
+ // (Required)
+ //
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // (Required) User context
+ UserContext user_context = 2;
+
+ // (Required) The operation for the config.
+ Operation operation = 3;
+
+ // Provides optional information about the client sending the request. This field
+ // can be used for language or version specific information and is only intended for
+ // logging purposes and will not be interpreted by the server.
+ optional string client_type = 4;
+
+ message Operation {
+ oneof op_type {
+ Set set = 1;
+ Get get = 2;
+ GetWithDefault get_with_default = 3;
+ GetOption get_option = 4;
+ GetAll get_all = 5;
+ Unset unset = 6;
+ IsModifiable is_modifiable = 7;
+ }
+ }
+
+ message Set {
+ // (Required) The config key-value pairs to set.
+ repeated KeyValue pairs = 1;
+ }
+
+ message Get {
+ // (Required) The config keys to get.
+ repeated string keys = 1;
+ }
+
+ message GetWithDefault {
+ // (Required) The config key-value paris to get. The value will be used as the default value.
+ repeated OptionalKeyValue pairs = 1;
+ }
+
+ message GetOption {
+ // (Required) The config keys to get optionally.
+ repeated string keys = 1;
+ }
+
+ message GetAll {
+ // (Optional) The prefix of the config key to get.
+ optional string prefix = 1;
+ }
+
+ message Unset {
+ // (Required) The config keys to unset.
+ repeated string keys = 1;
+ }
+
+ message IsModifiable {
+ // (Required) The config keys to check the config is modifiable.
+ repeated string keys = 1;
+ }
+}
+
+// Response to the config request.
+message ConfigResponse {
+ string client_id = 1;
+
+ // (Required) The operation for the config.
+ Operation operation = 2;
+
+ // (Optional)
+ //
+ // Warning messages for deprecated or unsupported configurations.
+ repeated string warnings = 3;
+
+ message Operation {
+ oneof op_type {
+ Set set = 1;
+ Get get = 2;
+ GetWithDefault get_with_default = 3;
+ GetOption get_option = 4;
+ GetAll get_all = 5;
+ Unset unset = 6;
+ IsModifiable is_modifiable = 7;
+ }
+ }
Review Comment:
Shouldn't the ConfigResponse simply be
```
repeated KeyValue values
```
?
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,141 @@ message ExecutePlanResponse {
}
}
+// The placeholder for the config request and response when the values can be optional.
+message OptionalValue {
+ optional string value = 1;
+}
+
+// The key-value pair for the config request and response.
+message KeyValue {
+ string key = 1;
+ string value = 2;
+}
+
+// The key-value pair for the config request and response when the value can be optional.
+message OptionalKeyValue {
+ string key = 1;
+ OptionalValue value = 2;
+}
+
Review Comment:
My suggestion would be to simply make the value optional in the KeyValue pair. Since proto does not have required fields per se, this avoids some of the repetion.
--
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
[GitHub] [spark] ueshin commented on a diff in pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
Posted by "ueshin (via GitHub)" <gi...@apache.org>.
ueshin commented on code in PR #40150:
URL: https://github.com/apache/spark/pull/40150#discussion_r1117409532
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,141 @@ message ExecutePlanResponse {
}
}
+// The placeholder for the config request and response when the values can be optional.
+message OptionalValue {
+ optional string value = 1;
+}
+
+// The key-value pair for the config request and response.
+message KeyValue {
+ string key = 1;
+ string value = 2;
+}
+
+// The key-value pair for the config request and response when the value can be optional.
+message OptionalKeyValue {
+ string key = 1;
+ OptionalValue value = 2;
+}
+
+// Request to update or fetch the configurations.
+message ConfigRequest {
+ // (Required)
+ //
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // (Required) User context
+ UserContext user_context = 2;
+
+ // (Required) The operation for the config.
+ Operation operation = 3;
+
+ // Provides optional information about the client sending the request. This field
+ // can be used for language or version specific information and is only intended for
+ // logging purposes and will not be interpreted by the server.
+ optional string client_type = 4;
+
+ message Operation {
+ oneof op_type {
+ Set set = 1;
+ Get get = 2;
+ GetWithDefault get_with_default = 3;
+ GetOption get_option = 4;
+ GetAll get_all = 5;
+ Unset unset = 6;
+ IsModifiable is_modifiable = 7;
+ }
+ }
+
+ message Set {
+ // (Required) The config key-value pairs to set.
+ repeated KeyValue pairs = 1;
+ }
+
+ message Get {
+ // (Required) The config keys to get.
+ repeated string keys = 1;
+ }
+
+ message GetWithDefault {
+ // (Required) The config key-value paris to get. The value will be used as the default value.
+ repeated OptionalKeyValue pairs = 1;
+ }
+
+ message GetOption {
+ // (Required) The config keys to get optionally.
+ repeated string keys = 1;
+ }
+
+ message GetAll {
+ // (Optional) The prefix of the config key to get.
+ optional string prefix = 1;
+ }
+
+ message Unset {
+ // (Required) The config keys to unset.
+ repeated string keys = 1;
+ }
+
+ message IsModifiable {
+ // (Required) The config keys to check the config is modifiable.
+ repeated string keys = 1;
+ }
+}
+
+// Response to the config request.
+message ConfigResponse {
+ string client_id = 1;
+
+ // (Required) The operation for the config.
+ Operation operation = 2;
+
+ // (Optional)
+ //
+ // Warning messages for deprecated or unsupported configurations.
+ repeated string warnings = 3;
+
+ message Operation {
+ oneof op_type {
+ Set set = 1;
+ Get get = 2;
+ GetWithDefault get_with_default = 3;
+ GetOption get_option = 4;
+ GetAll get_all = 5;
+ Unset unset = 6;
+ IsModifiable is_modifiable = 7;
+ }
+ }
Review Comment:
Let me just use `"true"` and `"false"` for now.
--
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
[GitHub] [spark] amaliujia commented on a diff in pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #40150:
URL: https://github.com/apache/spark/pull/40150#discussion_r1117500909
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,141 @@ message ExecutePlanResponse {
}
}
+// The placeholder for the config request and response when the values can be optional.
+message OptionalValue {
+ optional string value = 1;
+}
+
+// The key-value pair for the config request and response.
+message KeyValue {
+ string key = 1;
+ string value = 2;
+}
+
+// The key-value pair for the config request and response when the value can be optional.
+message OptionalKeyValue {
+ string key = 1;
+ OptionalValue value = 2;
+}
Review Comment:
Nit that please follow the proto style guide to mark each of the field by `(Required)` or `(Optional)`.
--
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
[GitHub] [spark] zhengruifeng commented on a diff in pull request #40150: [SPARK-41834][CONNECT] Implement SparkSession.conf
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #40150:
URL: https://github.com/apache/spark/pull/40150#discussion_r1116623834
##########
python/pyspark/sql/conf.py:
##########
@@ -35,12 +35,14 @@ def __init__(self, jconf: JavaObject) -> None:
self._jconf = jconf
@since(2.0)
- def set(self, key: str, value: str) -> None:
+ def set(self, key: str, value: Union[str, int, bool]) -> None:
"""Sets the given Spark runtime configuration property."""
Review Comment:
if we reuse existing doc in sql, we normally add a comment like this:
```
.. versionchanged:: 3.4.0
Support Spark Connect.
```
or maybe
```
.. versionchanged:: 4.0.0 (or 3.5.0?)
Support Spark Connect.
```
--
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