You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cdkrot (via GitHub)" <gi...@apache.org> on 2024/01/22 12:21:25 UTC
[PR] [SPARK-46792] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
cdkrot opened a new pull request, #44832:
URL: https://github.com/apache/spark/pull/44832
### What changes were proposed in this pull request?
Refactor ChannelBuilder into ChannelBuilder and DefaultChannelBuilder. ChannelBuilder is a abstract class with certain base functionality other possible channel builders should inherit. DefaultChannelBuilder is the current implementation of ChannelBuilder.
### Why are the changes needed?
The concept of ChannelBuilder was introduced specifically so that different implementations of Channel Builder would exist. However, the current version makes it hard to actually implement custom functionality, because the interface of channel builder and it's standard implementation are tied so closely.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests; new test TBD.
### Was this patch authored or co-authored using generative AI tooling?
Nope :)
--
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
Re: [PR] [SPARK-46792][PYTHON][CONNECT] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #44832:
URL: https://github.com/apache/spark/pull/44832#issuecomment-1907107113
Let's fix up the linter failure
--
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
Re: [PR] [SPARK-46792][PYTHON][CONNECT] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "cdkrot (via GitHub)" <gi...@apache.org>.
cdkrot commented on code in PR #44832:
URL: https://github.com/apache/spark/pull/44832#discussion_r1462999084
##########
python/pyspark/sql/connect/client/core.py:
##########
@@ -15,7 +15,8 @@
# limitations under the License.
#
__all__ = [
- "ChannelBuilder",
+ "DefaultChannelBuilder",
+ "DefaultChannelBuilder",
Review Comment:
Should have been "ChannelBuilder", "DefaultChannelBuilder", thanks!
--
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
Re: [PR] [SPARK-46792][PYTHON][CONNECT] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "cdkrot (via GitHub)" <gi...@apache.org>.
cdkrot commented on PR #44832:
URL: https://github.com/apache/spark/pull/44832#issuecomment-1908394205
Thanks! Tests green 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
Re: [PR] [SPARK-46792][PYTHON][CONNECT] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #44832:
URL: https://github.com/apache/spark/pull/44832#issuecomment-1909112709
Merged to master.
--
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
Re: [PR] [SPARK-46792] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44832:
URL: https://github.com/apache/spark/pull/44832#discussion_r1462575350
##########
python/pyspark/sql/connect/client/core.py:
##########
@@ -15,7 +15,8 @@
# limitations under the License.
#
__all__ = [
- "ChannelBuilder",
+ "DefaultChannelBuilder",
+ "DefaultChannelBuilder",
Review Comment:
remove one?
--
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
Re: [PR] [SPARK-46792][PYTHON][CONNECT] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #44832: [SPARK-46792][PYTHON][CONNECT] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder
URL: https://github.com/apache/spark/pull/44832
--
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
Re: [PR] [SPARK-46792] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44832:
URL: https://github.com/apache/spark/pull/44832#discussion_r1462575048
##########
python/pyspark/sql/connect/client/core.py:
##########
@@ -450,18 +486,18 @@ def keys(self) -> List[str]:
class AnalyzeResult:
def __init__(
- self,
- schema: Optional[DataType],
- explain_string: Optional[str],
- tree_string: Optional[str],
- is_local: Optional[bool],
- is_streaming: Optional[bool],
- input_files: Optional[List[str]],
- spark_version: Optional[str],
- parsed: Optional[DataType],
- is_same_semantics: Optional[bool],
- semantic_hash: Optional[int],
- storage_level: Optional[StorageLevel],
+ self,
Review Comment:
Hm, we won;t need formatting here. Was it by python auto formatter?
--
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
Re: [PR] [SPARK-46792] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44832:
URL: https://github.com/apache/spark/pull/44832#discussion_r1462575526
##########
python/pyspark/sql/connect/client/core.py:
##########
@@ -100,26 +101,183 @@ class ChannelBuilder:
This is a helper class that is used to create a GRPC channel based on the given
connection string per the documentation of Spark Connect.
+ The standard implementation is in DefaultChannelBuilder.
Review Comment:
```suggestion
The standard implementation is in :class:`DefaultChannelBuilder`.
```
--
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
Re: [PR] [SPARK-46792][PYTHON][CONNECT] Refactor ChannelBuilder into DefaultChannelBuilder and ChannelBuilder [spark]
Posted by "cdkrot (via GitHub)" <gi...@apache.org>.
cdkrot commented on code in PR #44832:
URL: https://github.com/apache/spark/pull/44832#discussion_r1463001550
##########
python/pyspark/sql/connect/client/core.py:
##########
@@ -450,18 +486,18 @@ def keys(self) -> List[str]:
class AnalyzeResult:
def __init__(
- self,
- schema: Optional[DataType],
- explain_string: Optional[str],
- tree_string: Optional[str],
- is_local: Optional[bool],
- is_streaming: Optional[bool],
- input_files: Optional[List[str]],
- spark_version: Optional[str],
- parsed: Optional[DataType],
- is_same_semantics: Optional[bool],
- semantic_hash: Optional[int],
- storage_level: Optional[StorageLevel],
+ self,
Review Comment:
Used autoformatter and it get back to reason, so probably IDE did. I'm changing it back, sorry :)
--
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