You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhengruifeng (via GitHub)" <gi...@apache.org> on 2024/03/26 08:25:42 UTC
[PR] [SPARK-47562][CONNECT] Factor literal handling out of `plan.py` [spark]
zhengruifeng opened a new pull request, #45719:
URL: https://github.com/apache/spark/pull/45719
### What changes were proposed in this pull request?
Factor literal handling out of `plan.py`
### Why are the changes needed?
we should not do the parameter pre-processing in `plan.py`
### Does this PR introduce _any_ user-facing change?
no, just refactoring
### How was this patch tested?
ci
### Was this patch authored or co-authored using generative AI tooling?
no
--
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-47562][CONNECT] Factor literal handling out of `plan.py` [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #45719:
URL: https://github.com/apache/spark/pull/45719#issuecomment-2021917795
thanks, 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-47562][CONNECT] Factor literal handling out of `plan.py` [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #45719:
URL: https://github.com/apache/spark/pull/45719#discussion_r1540281794
##########
python/pyspark/sql/connect/dataframe.py:
##########
@@ -1443,8 +1443,25 @@ def all_of_(xs: Iterable) -> bool:
message_parameters={},
)
+ def _convert_int_to_float(v: Any) -> Any:
Review Comment:
no need to do `_` prefix. it's nested func so not accessible
--
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-47562][CONNECT] Factor literal handling out of `plan.py` [spark]
Posted by "xinrong-meng (via GitHub)" <gi...@apache.org>.
xinrong-meng commented on PR #45719:
URL: https://github.com/apache/spark/pull/45719#issuecomment-2021129042
LGTM, thank you!
--
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-47562][CONNECT] Factor literal handling out of `plan.py` [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #45719:
URL: https://github.com/apache/spark/pull/45719#discussion_r1540291098
##########
python/pyspark/sql/connect/plan.py:
##########
@@ -1120,53 +1126,48 @@ def plan(self, session: "SparkConnectClient") -> proto.Relation:
class SQL(LogicalPlan):
- def __init__(self, query: str, args: Optional[Union[Dict[str, Any], List]] = None) -> None:
+ def __init__(
+ self,
+ query: str,
+ args: Optional[List[Column]] = None,
Review Comment:
good point, I roughly remember that @HyukjinKwon recommended use `Sequence` insead of `List`.
There are also other mix usage of `Mapping <-> Dict`, probably we can unify them in a separate PR
--
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-47562][CONNECT] Factor literal handling out of `plan.py` [spark]
Posted by "xinrong-meng (via GitHub)" <gi...@apache.org>.
xinrong-meng commented on code in PR #45719:
URL: https://github.com/apache/spark/pull/45719#discussion_r1540306336
##########
python/pyspark/sql/connect/plan.py:
##########
@@ -1120,53 +1126,48 @@ def plan(self, session: "SparkConnectClient") -> proto.Relation:
class SQL(LogicalPlan):
- def __init__(self, query: str, args: Optional[Union[Dict[str, Any], List]] = None) -> None:
+ def __init__(
+ self,
+ query: str,
+ args: Optional[List[Column]] = None,
Review Comment:
That's good to know, sounds good!
--
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-47562][CONNECT] Factor literal handling out of `plan.py` [spark]
Posted by "xinrong-meng (via GitHub)" <gi...@apache.org>.
xinrong-meng commented on code in PR #45719:
URL: https://github.com/apache/spark/pull/45719#discussion_r1539836619
##########
python/pyspark/sql/connect/plan.py:
##########
@@ -1120,53 +1126,48 @@ def plan(self, session: "SparkConnectClient") -> proto.Relation:
class SQL(LogicalPlan):
- def __init__(self, query: str, args: Optional[Union[Dict[str, Any], List]] = None) -> None:
+ def __init__(
+ self,
+ query: str,
+ args: Optional[List[Column]] = None,
Review Comment:
Irrevevant to changes proposed in this PR, I noticed a mix use of `List` and `Sequence` type hints in the file, but not necessarily indicating the mutability of variable.
--
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-47562][CONNECT] Factor literal handling out of `plan.py` [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng closed pull request #45719: [SPARK-47562][CONNECT] Factor literal handling out of `plan.py`
URL: https://github.com/apache/spark/pull/45719
--
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