[SPARK-2871] [PySpark] add key argument for max(), min() and top(n)

[davies]

RDD.max(key=None)

    @param key: A function used to generate key for comparing

    >>> rdd = sc.parallelize([1.0, 5.0, 43.0, 10.0])
    >>> rdd.max()
    43.0
    >>> rdd.max(key=str)
    5.0

RDD.min(key=None)

    Find the minimum item in this RDD.

    @param key: A function used to generate key for comparing

    >>> rdd = sc.parallelize([2.0, 5.0, 43.0, 10.0])
    >>> rdd.min()
    2.0
    >>> rdd.min(key=str)
    10.0

RDD.top(num, key=None)

    Get the top N elements from a RDD.

    Note: It returns the list sorted in descending order.
    >>> sc.parallelize([10, 4, 2, 12, 3]).top(1)
    [12]
    >>> sc.parallelize([2, 3, 4, 5, 6], 2).top(2)
    [6, 5]
    >>> sc.parallelize([10, 4, 2, 12, 3]).top(3, key=str)
    [4, 3, 2]

[SparkQA]

QA tests have started for PR 2094 at commit dd91e08.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2094 at commit dd91e08.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[mattf]

nit - the buildin 'max'

[davies]

I think cmp is the function used in max or min, so cmp is the default value for comp.

[mattf]

cmp may be used in max, but for this func the default is on line 829. either way, a minor nitpick.

[davies]

Yes, using comp here is bit confusing. The builtin min use key, it will be better for Python programer, but it will be different than Scala API.

cc @mateiz @rxin @JoshRosen

[davies]

We already use key in Python instead of Ordering in Scala, so I had change it into key.

Also , I would like to add key to top(), will be helpful, such as:

rdd.map(lambda x: (x, 1)).reduce(add).top(20, key=itemgetter(1))

We already have ord in Scala. Should I add this in this PR?

[mattf]

are you planning to add tests for these?

[davies]

@mattf thank you for reviewing this, I think the docs tests is enough, they have cover the cases w or w/o comp, which kinds of tests should be added?

[mattf]

consider default of comp=min in arg list and test for comp is not min

same for max method

[davies]

min and comp have different meanings:

>>> min(1, 2)
1
>>> cmp(1, 2)
-1

[mattf]

agreed re doctest. i forgot it was in use.

[SparkQA]

QA tests have started for PR 2094 at commit 2f63512.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2094 at commit 2f63512.

• This patch fails unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

QA tests have started for PR 2094 at commit ad7e374.

• This patch merges cleanly.

[SparkQA]

QA tests have started for PR 2094 at commit ccbaf25.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2094 at commit ccbaf25.

• This patch fails unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

QA tests have finished for PR 2094 at commit ad7e374.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[JoshRosen]

Epydoc renders docstrings + @params kind of oddly, but I don't think it's a big deal:

In the long run, we might want to move to Sphinx, since that seems to be what's popular with most major Python projects.

[JoshRosen]

I like this updated approach of using key instead of a comparator, since that's a closer match to Python's min function. Can you update the PR's title and description to reflect this?

[JoshRosen]

I've merged this into master. Thanks!