[GitHub] [spark] amaliujia commented on a diff in pull request #38393: [SPARK-40915][CONNECT] Improve `on` in Join in Python client
amaliujia commented on code in PR #38393: URL: https://github.com/apache/spark/pull/38393#discussion_r1009005272 ## python/pyspark/sql/connect/plan.py: ## @@ -575,7 +575,14 @@ def plan(self, session: Optional["RemoteSparkSession"]) -> proto.Relation: rel = proto.Relation() rel.join.left.CopyFrom(self.left.plan(session)) rel.join.right.CopyFrom(self.right.plan(session)) -rel.join.join_condition.CopyFrom(self.to_attr_or_expression(self.on, session)) +if self.on is not None: +if not isinstance(self.on, list): +if isinstance(self.on, str): Review Comment: If you check existing PySpark implementation, yes, this is what I learn from there to support such multiple types. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #38393: [SPARK-40915][CONNECT] Improve `on` in Join in Python client
amaliujia commented on code in PR #38393: URL: https://github.com/apache/spark/pull/38393#discussion_r1008513991 ## python/pyspark/sql/connect/plan.py: ## @@ -537,7 +537,7 @@ def __init__( self, left: Optional["LogicalPlan"], right: "LogicalPlan", -on: "ColumnOrString", +on: Optional[Union[str, List[str], ColumnRef]], Review Comment: hmmm I gave a try but failed to make it work for mypy and UT together (not a python expert): I found the trick case is `str` seems to be also consider-red as `Sequence` or `Iterable` which have caused things for implementation -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #38393: [SPARK-40915][CONNECT] Improve `on` in Join in Python client
amaliujia commented on code in PR #38393: URL: https://github.com/apache/spark/pull/38393#discussion_r1008513991 ## python/pyspark/sql/connect/plan.py: ## @@ -537,7 +537,7 @@ def __init__( self, left: Optional["LogicalPlan"], right: "LogicalPlan", -on: "ColumnOrString", +on: Optional[Union[str, List[str], ColumnRef]], Review Comment: hmmm I gave a try but failed to make it work for mypy and UT together (not a python expert): I found the trick case is `str` seems to be also consider-red as `Sequence` or `Iterable`. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #38393: [SPARK-40915][CONNECT] Improve `on` in Join in Python client
amaliujia commented on code in PR #38393: URL: https://github.com/apache/spark/pull/38393#discussion_r1006437955 ## python/pyspark/sql/connect/plan.py: ## @@ -537,7 +537,7 @@ def __init__( self, left: Optional["LogicalPlan"], right: "LogicalPlan", -on: "ColumnOrString", +on: Optional[Union[str, List[str], ColumnRef]], Review Comment: Is `list` to `Sequence` or `Iterable[xxx]` a API incompatible change? If not I can try to fix in this PR which sounds straightforward. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #38393: [SPARK-40915][CONNECT] Improve `on` in Join in Python client
amaliujia commented on code in PR #38393: URL: https://github.com/apache/spark/pull/38393#discussion_r1006436736 ## python/pyspark/sql/connect/dataframe.py: ## @@ -218,8 +218,13 @@ def head(self, n: int) -> Optional["pandas.DataFrame"]: self.limit(n) return self.toPandas() -# TODO(martin.grund) fix mypu -def join(self, other: "DataFrame", on: Any, how: Optional[str] = None) -> "DataFrame": +# TODO: extend `on` to also be type List[ColumnRef]. Review Comment: Join condition could be something like `[df.name=df2.name, df.age=df2.age]` which will be mapped to List[ColumnRef]. Current expression system in Connect is not perfect that we cannot convert the expression above into a simple expression. There are mixed ColumnRef, String, Expression, List, etc. and we need to figure out a way to unify all of those into ultimately one single expression. If that is too hard then at least we need to change proto to allow accepting a list of Expression. I will follow up on the expression support in the 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
