You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by HyukjinKwon <gi...@git.apache.org> on 2018/02/03 18:33:14 UTC

[GitHub] spark pull request #20499: [SPARK-19454][PYTHON][FOLLOWUP] Disallow default ...

GitHub user HyukjinKwon opened a pull request:

    https://github.com/apache/spark/pull/20499

    [SPARK-19454][PYTHON][FOLLOWUP] Disallow default value None when 'to_replace' is not a dictionary

    ## What changes were proposed in this pull request?
    
    This PR proposes to disallow default value None when 'to_replace' is not a dictionary.
    
    It seems weird we set the default value of `value` to `None` and we ended up allowing the case as below:
    
    ```python
    >>> df.show()
    ```
    ```
    +----+------+-----+
    | age|height| name|
    +----+------+-----+
    |  10|    80|Alice|
    ...
    ```
    
    ```python
    >>> df.na.replace('Alice').show()
    ```
    ```
    +----+------+----+
    | age|height|name|
    +----+------+----+
    |  10|    80|null|
    ...
    ```
    
    **After**
    
    This PR targets to disallow the case above:
    
    ```python
    >>> df.na.replace('Alice').show()
    ```
    ```
    ...
    TypeError: value is required when to_replace is not a dictionary.
    ```
    
    while we still allow when `to_replace` is a dictionary:
    
    ```python
    >>> df.na.replace({'Alice': None}).show()
    ```
    ```
    +----+------+----+
    | age|height|name|
    +----+------+----+
    |  10|    80|null|
    ...
    ```
    
    This is tricky because we should `value` is explicitly set or not. So, this PR uses `args` and `kwargs`.
    
    So, there are manual arguments checks:
    
    ```
    >>> df.na.replace(1, 2, 3, 4).show()
    ...
    TypeError: Arguments are expected to be to_replace, value and subset; however, got [1, 2, 3, 4]
    ```
    
    ```
    >>> df.na.replace("a", foo=1, bar=1).show()
    ...
    TypeError: Arguments are expected to be to_replace, value and subset; however, got unexpected keyword arguments: [foo, bar]
    ```
    
    One small downside of this is pydoc because now we use args and kwargs signature:
    
    ```
    replace(self, to_replace, *args, **kwargs)
    ```
    
    ## How was this patch tested?
    
    Manually tested, tests were added in `python/pyspark/sql/tests.py` and doctests were fixed.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/HyukjinKwon/spark SPARK-19454-followup

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/20499.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #20499
    
----
commit 13bdc24696563a85283a9bb6d9dc3f9bb2bedaf2
Author: hyukjinkwon <gu...@...>
Date:   2018-02-03T18:14:04Z

    Disallow default value None when 'to_replace' is not a dictionary

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/706/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20499: [SPARK-23328][PYTHON] Disallow default value None...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20499#discussion_r165834947
  
    --- Diff: python/pyspark/sql/dataframe.py ---
    @@ -1587,6 +1600,51 @@ def replace(self, to_replace, value=None, subset=None):
             |null|  null|null|
             +----+------+----+
             """
    +
    +        # It deals with a problem when 'value' is set to None and 'to_replace' is a dictionary.
    +        is_more_than_two = len(args) + len(kwargs) > 2
    +        if is_more_than_two:
    +            raise TypeError(
    +                "Arguments are expected to be to_replace, value and subset; however, got "
    +                "[%s]" % ", ".join(map(str, [to_replace] + list(args) + list(kwargs.values()))))
    +
    +        is_unexpected_kwargs = \
    +            len(kwargs) == 2 and ("value" not in kwargs or "subset" not in kwargs)
    +        if is_unexpected_kwargs:
    +            raise TypeError(
    +                "Arguments are expected to be to_replace, value and subset; however, got "
    +                "unexpected keyword arguments: [%s]" % ", ".join(
    +                    filter(lambda a: a not in ["value", "subset"], kwargs.keys())))
    +
    +        if "value" in kwargs and "subset" in kwargs:
    +            # replace(..., value=..., subset=...)
    +            # replace(..., subset=..., value=...)
    +            value = kwargs["value"]
    +            subset = kwargs["subset"]
    +        elif "value" in kwargs:
    +            # replace(..., ..., value=...)
    +            value = kwargs["value"]
    +            subset = args[0] if len(args) == 1 else None
    +        elif "subset" in kwargs:
    +            # replace(..., ..., subset=...)
    +            if len(args) == 1:
    +                value = args[0]
    +            elif isinstance(to_replace, dict):
    +                value = None  # When to_replace is a dictionary, value can be omitted.
    +            else:
    +                raise TypeError("value is required when to_replace is not a dictionary.")
    --- End diff --
    
    Hm, I think that check is still valid. The newly added logic here focuses on checking missing arguments whereas the below logics focus on checking if arguments are valid.
    
    Will try to add a explicit test for https://github.com/apache/spark/pull/20499#discussion_r165828448 case with few comment changes.
    
    For https://github.com/apache/spark/pull/20499#discussion_r165828519, I just tried to check. Seems we should keep that `None` to support:
    
    ```python
    >>> df.na.replace('Alice', None).show()
    ```
    
    ```
    +----+------+----+
    | age|height|name|
    +----+------+----+
    |  10|    80|null|
    ...
    ```
    
    ```
    ...
    ValueError: If to_replace is not a dict, value should be a bool, float, int, long, string, list, tuple or None. Got <type 'NoneType'>
    ```


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    Will update this tonight


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    Yup, I should fix the guide for 2.2 anyway :-) Will open a backport tonight KST.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    **[Test build #87205 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87205/testReport)** for PR 20499 at commit [`885b4d0`](https://github.com/apache/spark/commit/885b4d00af53dfd0148c431fdacce9a2789f32a2).
     * This patch **fails PySpark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    Yup, sounds good.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    **[Test build #87051 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87051/testReport)** for PR 20499 at commit [`5c18594`](https://github.com/apache/spark/commit/5c185941312d5379761d7600127c735eb5b27084).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    **[Test build #87206 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87206/testReport)** for PR 20499 at commit [`885b4d0`](https://github.com/apache/spark/commit/885b4d00af53dfd0148c431fdacce9a2789f32a2).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    Sure, let me unset the target version.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by viirya <gi...@git.apache.org>.

Github user viirya commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    I think that behavior is shipped in 2.2, right? Then we may need to add a note in migration guide.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20499: [SPARK-23328][PYTHON] Disallow default value None in na....

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20499
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org