You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/04/26 07:11:26 UTC
[GitHub] [spark] Yikun opened a new pull request, #36353: [SPARK-38946][PYTHON] Fix different behavior in setitem
Yikun opened a new pull request, #36353:
URL: https://github.com/apache/spark/pull/36353
### What changes were proposed in this pull request?
Since pandas 1.4.0 https://github.com/pandas-dev/pandas/commit/03dd698bc1e84c35aba8b51bdd45c472860b9ec3 , setitem should always make a copy and never write into the existing array.
But for pandas on Spark, https://github.com/databricks/koalas/pull/1592 , the setitem refer the anchor DataFrame.
### Why are the changes needed?
Make CI passed in with pandas 1.4.x
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
CI test with current pandas and 1.4.x:
Especially,
```
DataFrameTest.test_eval
DataFrameTest.test_update
DataFrameTest.test_inplace
DataFrameTest.test_fillna
```
--
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 #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1140606896
Let me defer to @ueshin
--
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] itholic commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r881433388
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
What if `requires_same_anchor` and `force_disconnect` are both `True` ?
--
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 #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
URL: https://github.com/apache/spark/pull/36353
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1135943774
Test with Panda 1.4.2: https://github.com/Yikun/spark/runs/6574547715?check_suite_focus=true
After this PR, we can upgrade pandas to 1.4.2.
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r881456319
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
This patch is more about adding some special processing to follow pandas behavior, `__setitems__` generate the new df after pandas 1.4, some API behaviors changes.
So, if `force_disconnect` is `True` always return new df otherwise keep the original logic.
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883205995
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
If `requires_same_anchor` is `False`, checking whether or not the same anchor is ignored
and force to update the InternalFrame, e.g., replacing the internal with the resolved_copy,
updating the underlying Spark DataFrame which need to combine a different Spark DataFrame.
- :param internal: the new InternalFrame
- :param requires_same_anchor: whether checking the same anchor
+ Parameters
+ ----------
+ internal : InternalFrame
+ The new InternalFrame
+ requires_same_anchor : bool
+ Whether checking the same anchor
+ force_disconnect : bool
+ Force to disconnect the link to Series and create new one
Review Comment:
```suggestion
anchor_force_disconnect : bool
Force to disconnect the original anchor and create new one
```
How about? : )
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1217816080
Just a rebase
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1183864465
just a rebase, because we update numpy version in infra
--
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 #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
ueshin commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r886205809
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -2910,7 +2920,12 @@ def get_data(left_columns=None, right_columns=None):
left_pdf.update(right_pdf)
left_psdf.update(right_psdf)
self.assert_eq(left_pdf.sort_values(by=["A", "B"]), left_psdf.sort_values(by=["A", "B"]))
- self.assert_eq(psser.sort_index(), pser.sort_index())
+ # SPARK-38946: Since Spark 3.4, df.update generate a new dataframe to follow
+ # pandas 1.4 behaviors
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(psser.sort_index(), pser.sort_index())
Review Comment:
ditto.
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -270,7 +270,12 @@ def test_inplace(self):
psdf["a"] = psdf["a"] + 10
self.assert_eq(psdf, pdf)
- self.assert_eq(psser, pser)
+ # SPARK-38946: Since Spark 3.4, inplace set generate a new dataframe to follow
+ # pandas 1.4 behaviors
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(psser, pser)
Review Comment:
Seems like this is a common test with the old pandas?
Shall we move it out of `if LooseVersion(pd.__version__) >= LooseVersion("1.4"):`?
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -1449,7 +1454,12 @@ def test_fillna(self):
pdf.fillna({"x": -1, "y": -2, "z": -5}, inplace=True)
psdf.fillna({"x": -1, "y": -2, "z": -5}, inplace=True)
self.assert_eq(psdf, pdf)
- self.assert_eq(psser, pser)
+ # SPARK-38946: Since Spark 3.4, fillna with inplace generate a new dataframe to follow
+ # pandas 1.4 behaviors
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(psser, pser)
Review Comment:
ditto.
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -5151,7 +5166,11 @@ def test_eval(self):
pdf.eval("A = B + C", inplace=True)
psdf.eval("A = B + C", inplace=True)
self.assert_eq(pdf, psdf)
- self.assert_eq(pser, psser)
+ # SPARK-38946: Since Pandas 1.4, eval with inplace generate a new dataframe
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(pser, psser)
Review Comment:
ditto.
--
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] xinrong-databricks commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
xinrong-databricks commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883193002
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
If `requires_same_anchor` is `False`, checking whether or not the same anchor is ignored
and force to update the InternalFrame, e.g., replacing the internal with the resolved_copy,
updating the underlying Spark DataFrame which need to combine a different Spark DataFrame.
- :param internal: the new InternalFrame
- :param requires_same_anchor: whether checking the same anchor
+ Parameters
+ ----------
+ internal : InternalFrame
+ The new InternalFrame
+ requires_same_anchor : bool
+ Whether checking the same anchor
+ force_disconnect : bool
+ Force to disconnect the link to Series and create new one
Review Comment:
What do you think we call out `force_disconnect` takes effects on anchor frames only, or rename it with `anchor` included? My concern is that the word `Series` here may seem fuzzy.
--
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] xinrong-databricks commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
xinrong-databricks commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883193494
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
Is keeping version checks in unit tests, but no version checks in main codes a good idea?
--
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 #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1179881408
I will leave it to @ueshin
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1165688386
@ueshin ready for review, it would be good if you could find some time. : )
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1211460815
@ueshin Thanks for review!
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883183319
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
Yep, this is a good question. Now it's more for making CI passed with current pandas version 1.3.x.
I think we should remove these code after we upgrade to 1.4.x. WDYT?
--
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] xinrong-databricks commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
xinrong-databricks commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883180986
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
Spark 3.4 is claimed to match pandas 1.4, do we need this version check?
--
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] itholic commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r882216133
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
Ah,,, I got it.
When I first see the name `requires_same_anchor`, I thought it means they have must same anchor between old and new `InternalFrame`, but seems like it's just for checking if they have same anchor ??
If so, how about rename the parameter to `check_same_anchor` or `verify_same_anchor` or something ?
I'm not very strong feeling about this, but I think maybe it's more proper naming.
WDYT?
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON] Fix different behavior in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1109434513
This is still a WIP PR now, only make ci passed with panda 1.4.x.
- If we want to follow pandas behavior, we might want to revert https://github.com/databricks/koalas/pull/1592
- If we only want to still keep this, we might only need this test fix 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
[GitHub] [spark] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1164343056
According to https://github.com/pandas-dev/pandas/issues/47188 and https://github.com/pandas-dev/pandas/issues/47449 , we should only address `set_item` in here, so just skip test for oldest 1.4.x pandas.
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1211914202
@HyukjinKwon Mind to take another look?
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1178956945
Let me do a brief conclusion to help review:
- **Change `setitem` to make a copy**https://github.com/pandas-dev/pandas/commit/03dd698bc1e84c35aba8b51bdd45c472860b9ec3 dataframe.setitem make a copy and never write into the existing array, so we also follow this.
- **Skip update/fillna for 1.4.0-1.4.2**: https://github.com/pandas-dev/pandas/issues/47188 this issue fix the `update` and `fillna` behaviors, I confirm it had been resolved by pandas 1.4.3, so we skip the validate copy check.
- **Skip eval for 1.4.0-1.4.3**: https://github.com/pandas-dev/pandas/issues/47449 `eval` still have a regression, so we skip it in 1.4.0~1.4.3.
I also test 1.4.2 CI with this PR: https://github.com/Yikun/spark/pull/113, all tests passed, so we can upgrade pandas to `1.4.2` in CI after this PR merged.
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r920652980
##########
python/pyspark/pandas/frame.py:
##########
@@ -8412,7 +8430,11 @@ def update(self, other: "DataFrame", join: str = "left", overwrite: bool = True)
*HIDDEN_COLUMNS,
)
internal = self._internal.with_new_sdf(sdf, data_fields=data_fields)
- self._update_internal_frame(internal, requires_same_anchor=False)
+ # Since Spark 3.4, df.update generates a new dataframe instead of operating
+ # in-place to follow pandas v1.4 behavior, see also SPARK-38946.
+ self._update_internal_frame(
+ internal, requires_same_anchor=False, anchor_force_disconnect=True
Review Comment:
See https://github.com/apache/spark/pull/36353#issuecomment-1178956945 , after confirm from panda community, we only add `setitem` make copy. so this line had been removed
##########
python/pyspark/pandas/frame.py:
##########
@@ -11944,7 +11966,11 @@ def eval_func(pdf): # type: ignore[no-untyped-def]
if inplace:
# Here, the result is always a frame because the error is thrown during schema inference
# from pandas.
- self._update_internal_frame(result._internal, requires_same_anchor=False)
+ # Since Spark 3.4, eval with inplace generates a new dataframe instead of operating
+ # in-place to follow pandas v1.4 behavior, see also SPARK-38946.
+ self._update_internal_frame(
+ result._internal, requires_same_anchor=False, anchor_force_disconnect=True
Review Comment:
See https://github.com/apache/spark/pull/36353#issuecomment-1178956945 , after confirm from panda community, we only add `setitem` make copy. so this line had been removed
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883238273
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
https://issues.apache.org/jira/browse/SPARK-39310 create a jira
--
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] itholic commented on pull request #36353: [SPARK-38946][PYTHON] Fix different behavior in setitem
Posted by GitBox <gi...@apache.org>.
itholic commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1135358115
Just confirming, any update on this ?
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r882223776
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
Yep, good suggestion, but `required_same_anchor` is orignal parameter, and already being used in current code.
Consider it's inertnal function, we could change it. I can submit a separate PR to address it to make PR easy to review.
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883230084
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
After some consideration, I think `keeping version checks in unit tests` is better, and also a better principle for similar cases:
- Don't use version check in code/implementation as far as possible, UT is a better place to do it, according UT we can easy to see what the behavior change.
- All test should be passed with any version of pandas > 1.0.5.
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r904950232
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -270,7 +270,12 @@ def test_inplace(self):
psdf["a"] = psdf["a"] + 10
self.assert_eq(psdf, pdf)
- self.assert_eq(psser, pser)
+ # SPARK-38946: Since Spark 3.4, inplace set generate a new dataframe to follow
+ # pandas 1.4 behaviors
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(psser, pser)
Review Comment:
we follow latest behavior, so before 1.3 will raise not equal.
--
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 #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
ueshin commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r886192259
##########
python/pyspark/pandas/frame.py:
##########
@@ -8412,7 +8430,11 @@ def update(self, other: "DataFrame", join: str = "left", overwrite: bool = True)
*HIDDEN_COLUMNS,
)
internal = self._internal.with_new_sdf(sdf, data_fields=data_fields)
- self._update_internal_frame(internal, requires_same_anchor=False)
+ # Since Spark 3.4, df.update generates a new dataframe instead of operating
+ # in-place to follow pandas v1.4 behavior, see also SPARK-38946.
+ self._update_internal_frame(
+ internal, requires_same_anchor=False, anchor_force_disconnect=True
Review Comment:
We don't need to specify `requires_same_anchor` any more if `anchor_force_disconnect=True`?
##########
python/pyspark/pandas/frame.py:
##########
@@ -11944,7 +11966,11 @@ def eval_func(pdf): # type: ignore[no-untyped-def]
if inplace:
# Here, the result is always a frame because the error is thrown during schema inference
# from pandas.
- self._update_internal_frame(result._internal, requires_same_anchor=False)
+ # Since Spark 3.4, eval with inplace generates a new dataframe instead of operating
+ # in-place to follow pandas v1.4 behavior, see also SPARK-38946.
+ self._update_internal_frame(
+ result._internal, requires_same_anchor=False, anchor_force_disconnect=True
Review Comment:
ditto.
--
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 pull request #36353: [SPARK-38946][PYTHON] Fix different behavior in setitem
Posted by GitBox <gi...@apache.org>.
ueshin commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1110297356
The examples in the description of https://github.com/databricks/koalas/pull/1592 are still valid.
E.g.,:
```py
>>> pd.__version__
'1.4.1'
>>> pdf = pd.DataFrame({"x": [np.nan, 2, 3, 4, np.nan, 6], "y": [np.nan, 2, 3, 4, np.nan, 6]})
>>> pser = pdf.x
>>> pser.fillna(0, inplace=True)
>>> pser
0 0.0
1 2.0
2 3.0
3 4.0
4 0.0
5 6.0
Name: x, dtype: float64
>>> pdf
x y
0 0.0 NaN
1 2.0 2.0
2 3.0 3.0
3 4.0 4.0
4 0.0 NaN
5 6.0 6.0
```
If we revert the change, it won't work anymore.
Have you tried to revert the changes except for the tests and run the whole tests?
If there are behavior changes in pandas, we should fix our behavior.
> I haven't got enough info to see why we added https://github.com/databricks/koalas/pull/1592 before (performance or follow pandas old behavior).
That's to follow the pandas behavior at that time and seems like it's still necessary.
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON] Fix different behavior in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1110982076
@ueshin OK, thanks for your info, I will take an another look soon.
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883205995
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
If `requires_same_anchor` is `False`, checking whether or not the same anchor is ignored
and force to update the InternalFrame, e.g., replacing the internal with the resolved_copy,
updating the underlying Spark DataFrame which need to combine a different Spark DataFrame.
- :param internal: the new InternalFrame
- :param requires_same_anchor: whether checking the same anchor
+ Parameters
+ ----------
+ internal : InternalFrame
+ The new InternalFrame
+ requires_same_anchor : bool
+ Whether checking the same anchor
+ force_disconnect : bool
+ Force to disconnect the link to Series and create new one
Review Comment:
```suggestion
anchor_force_disconnect : bool
Force to disconnect the original anchor and update with a new creating dataframe
```
How about? : )
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r904950232
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -270,7 +270,12 @@ def test_inplace(self):
psdf["a"] = psdf["a"] + 10
self.assert_eq(psdf, pdf)
- self.assert_eq(psser, pser)
+ # SPARK-38946: Since Spark 3.4, inplace set generate a new dataframe to follow
+ # pandas 1.4 behaviors
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(psser, pser)
Review Comment:
we follow latest behavior, so before 1.3 will raise not equal. added
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r940993384
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -1493,6 +1500,15 @@ def test_fillna(self):
pdf.fillna({"x": -1, "y": -2, "z": -5}, inplace=True)
psdf.fillna({"x": -1, "y": -2, "z": -5}, inplace=True)
self.assert_eq(psdf, pdf)
+ # Skip due to pandas bug: https://github.com/pandas-dev/pandas/issues/47188
+ if not (LooseVersion("1.4.0") <= LooseVersion(pd.__version__) <= LooseVersion("1.4.2")):
+ self.assert_eq(psser, pser)
+
+ pser = pdf.z
+ psser = psdf.z
+ pdf.fillna(0, inplace=True)
Review Comment:
Add a testcase which @ueshin mentioned before
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -1536,14 +1552,15 @@ def test_fillna(self):
self.assert_eq(pdf.fillna(method="bfill"), psdf.fillna(method="bfill"))
self.assert_eq(pdf.fillna(method="bfill", limit=2), psdf.fillna(method="bfill", limit=2))
- self.assert_eq(psdf.fillna({"x": -1}), pdf.fillna({"x": -1}))
-
- self.assert_eq(
- psdf.fillna({"x": -1, ("x", "b"): -2}), pdf.fillna({"x": -1, ("x", "b"): -2})
- )
- self.assert_eq(
- psdf.fillna({("x", "b"): -2, "x": -1}), pdf.fillna({("x", "b"): -2, "x": -1})
- )
+ # See also: https://github.com/pandas-dev/pandas/issues/47649
+ if LooseVersion("1.4.3") != LooseVersion(pd.__version__):
Review Comment:
Add a test to make sure 1.4.3 pass. it doesn't casue by this patch, it is a regression since pandas 1.4.3.
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1209973469
All test passed and
Test passed with Pandas 1.4.2:
https://github.com/Yikun/spark/pull/148
Test passed with Pandas 1.4.3:
https://github.com/Yikun/spark/pull/147
@ueshin Would you mind take a look? 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
[GitHub] [spark] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883230084
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
I think `keeping version checks in unit tests` is better, and also a better principle for similar cases:
- Don't use version check in code/implementation as far as possible, UT is a better place to do it, according UT we can easy to see what the behavior change.
- All test should be passed with any version of pandas > 1.0.5.
--
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] itholic commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883200198
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
If `requires_same_anchor` is `False`, checking whether or not the same anchor is ignored
and force to update the InternalFrame, e.g., replacing the internal with the resolved_copy,
updating the underlying Spark DataFrame which need to combine a different Spark DataFrame.
- :param internal: the new InternalFrame
- :param requires_same_anchor: whether checking the same anchor
+ Parameters
+ ----------
+ internal : InternalFrame
+ The new InternalFrame
+ requires_same_anchor : bool
+ Whether checking the same anchor
+ force_disconnect : bool
+ Force to disconnect the link to Series and create new one
Review Comment:
> What do you think we call out `force_disconnect` takes effects on anchor frames only, or rename it with `anchor` included? My concern is that the word `Series` here may seem fuzzy.
+1
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883183319
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
Yep, this is a good question. Now it's more for make CI passed with current pandas version 1.3.x.
I think we should remove these code after we upgrade to 1.4.x. WDYT?
--
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] xinrong-databricks commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
xinrong-databricks commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883181537
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
It seems fine to keep it. I am just wondering about our principle for similar cases.
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1143313995
https://github.com/pandas-dev/pandas/issues/47188
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883230765
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
If `requires_same_anchor` is `False`, checking whether or not the same anchor is ignored
and force to update the InternalFrame, e.g., replacing the internal with the resolved_copy,
updating the underlying Spark DataFrame which need to combine a different Spark DataFrame.
- :param internal: the new InternalFrame
- :param requires_same_anchor: whether checking the same anchor
+ Parameters
+ ----------
+ internal : InternalFrame
+ The new InternalFrame
+ requires_same_anchor : bool
+ Whether checking the same anchor
+ force_disconnect : bool
+ Force to disconnect the link to Series and create new one
Review Comment:
updated. Pls let me know if you have better idea.
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883230084
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
After some consideration, I think `keeping version checks in unit tests` is better, and also a better principle for similar cases:
- Don't use version check in code/implementation as far as possible, UT is a better place to do it, according UT we can easy to see what the behavior change.
- All test should be passed with any version of pandas > 1.0.5.
I have update the PR! thanks for discussion!
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r881448119
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
Thanks for review, not sure get your idea completely.
It's A or B or C, Create new dataframe if one of three coditions statified, so `if requires_same_anchor and force_disconnect are both True` also create the new dataframe.
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
Thanks for review, not sure got your idea completely.
It's A or B or C, Create new dataframe if one of three coditions statified, so `if requires_same_anchor and force_disconnect are both True` also create the new dataframe.
--
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] itholic commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r881439726
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
Oh, sorry. approve is a mistake.
But looks pretty good otherwise.
--
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 #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1219005321
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
[GitHub] [spark] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r920653546
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -1449,7 +1454,12 @@ def test_fillna(self):
pdf.fillna({"x": -1, "y": -2, "z": -5}, inplace=True)
psdf.fillna({"x": -1, "y": -2, "z": -5}, inplace=True)
self.assert_eq(psdf, pdf)
- self.assert_eq(psser, pser)
+ # SPARK-38946: Since Spark 3.4, fillna with inplace generate a new dataframe to follow
+ # pandas 1.4 behaviors
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(psser, pser)
Review Comment:
See https://github.com/apache/spark/pull/36353#issuecomment-1178956945 , change the version to 1.4.0-1.4.2
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -2910,7 +2920,12 @@ def get_data(left_columns=None, right_columns=None):
left_pdf.update(right_pdf)
left_psdf.update(right_psdf)
self.assert_eq(left_pdf.sort_values(by=["A", "B"]), left_psdf.sort_values(by=["A", "B"]))
- self.assert_eq(psser.sort_index(), pser.sort_index())
+ # SPARK-38946: Since Spark 3.4, df.update generate a new dataframe to follow
+ # pandas 1.4 behaviors
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(psser.sort_index(), pser.sort_index())
Review Comment:
See https://github.com/apache/spark/pull/36353#issuecomment-1178956945 , change the version to 1.4.0-1.4.2
##########
python/pyspark/pandas/tests/test_dataframe.py:
##########
@@ -5151,7 +5166,11 @@ def test_eval(self):
pdf.eval("A = B + C", inplace=True)
psdf.eval("A = B + C", inplace=True)
self.assert_eq(pdf, psdf)
- self.assert_eq(pser, psser)
+ # SPARK-38946: Since Pandas 1.4, eval with inplace generate a new dataframe
+ if LooseVersion(pd.__version__) >= LooseVersion("1.4"):
+ self.assert_eq(pser, psser)
Review Comment:
See https://github.com/apache/spark/pull/36353#issuecomment-1178956945 , change the version to 1.4.0-1.4.2
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883183319
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
Yep, this is a good question. Now it's more for making CI passed with current pandas version 1.3.x.
I thought we should remove these code after infra's pandas upgrade to 1.4.x. WDYT?
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r881448119
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
Thanks for review, not sure got your idea completely.
It's A or B or C, Create new dataframe if one of three coditions satisfied, so `if requires_same_anchor and force_disconnect are both True` also create the new dataframe.
--
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] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r882223776
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
Review Comment:
Yep, good suggestion, but `required_same_anchor` is orignal parameter, and already being used in current code.
Consider it's internal function, we could change it. I can submit a separate PR to address it to make PR easy to review.
--
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] xinrong-databricks commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
xinrong-databricks commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1139762141
The renaming is so much better, thanks Yikun! LGTM.
--
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] xinrong-databricks commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
xinrong-databricks commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883193792
##########
python/pyspark/pandas/frame.py:
##########
@@ -5495,7 +5506,15 @@ def op(psser: ps.Series) -> ps.Series:
inplace = validate_bool_kwarg(inplace, "inplace")
if inplace:
- self._update_internal_frame(psdf._internal, requires_same_anchor=False)
+ # Since Pandas 1.4, df.fillna generates a new dataframe instead of operating
+ # in-place. See also SPARK-38946. Currently, `bfill` and `ffill` still keep
+ # inplace operate behaviors.
+ force_disconnect = (method is None) and LooseVersion(pd.__version__) >= LooseVersion(
Review Comment:
Ohh sorry didn't refresh the page to see your comment. 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
[GitHub] [spark] Yikun commented on a diff in pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #36353:
URL: https://github.com/apache/spark/pull/36353#discussion_r883205869
##########
python/pyspark/pandas/frame.py:
##########
@@ -490,20 +491,30 @@ def _internal(self) -> InternalFrame:
return cast(InternalFrame, self._internal_frame) # type: ignore[has-type]
def _update_internal_frame(
- self, internal: InternalFrame, requires_same_anchor: bool = True
+ self,
+ internal: InternalFrame,
+ requires_same_anchor: bool = True,
+ force_disconnect: bool = False,
) -> None:
"""
Update InternalFrame with the given one.
- If the column_label is changed or the new InternalFrame is not the same `anchor`,
- disconnect the link to the Series and create a new one.
+ If the column_label is changed or the new InternalFrame is not the same `anchor` or the
+ `force_disconnect` flag is set to True, disconnect the link to the Series and create a new
+ one.
If `requires_same_anchor` is `False`, checking whether or not the same anchor is ignored
and force to update the InternalFrame, e.g., replacing the internal with the resolved_copy,
updating the underlying Spark DataFrame which need to combine a different Spark DataFrame.
- :param internal: the new InternalFrame
- :param requires_same_anchor: whether checking the same anchor
+ Parameters
+ ----------
+ internal : InternalFrame
+ The new InternalFrame
+ requires_same_anchor : bool
+ Whether checking the same anchor
+ force_disconnect : bool
+ Force to disconnect the link to Series and create new one
Review Comment:
```suggestion
anchor_force_disconnect : bool
Force to disconnect the original anchor and create new one
```
How about? : )
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON] Fix different behavior in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1135376632
@itholic We might want to do some special process when calling `_update_internal_frame` for pandas 1.4.x. Will update today.
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON][PS] Generates a new dataframe instead of operating inplace in df.eval/update/fillna/setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1143090488
> The condition to generate a new dataframe seems a bit more complex?
@ueshin Yes, it is really yes from your example, the `setitem` has behavior influence on several functions and change the final or part of behaviors for these functions (but pandas not mention this behavior change). Anyway, I will raise a issue on Pandas comunity to get the detail attitude for these beahavior changes.
--
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] Yikun commented on pull request #36353: [SPARK-38946][PYTHON] Fix different behavior in setitem
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1109434904
I haven't got enough info to see why we added https://github.com/databricks/koalas/pull/1592 before (performance or follow pandas old behavior).
cc @HyukjinKwon @ueshin Maybe you could give some input or suggestion in here.
--
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 #36353: [SPARK-38946][PYTHON] Fix different behavior in setitem
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #36353:
URL: https://github.com/apache/spark/pull/36353#issuecomment-1109616602
I think this needs @ueshin's look
--
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