Skip to content

Conversation

@udoklein
Copy link
Contributor

@udoklein udoklein commented Jan 7, 2016

According to the documentation the sortByKey method does not take a lambda as an argument, thus the example is flawed. Removed the argument completely as this will default to ascending sort.

According to the documentation the sortByKey method does not take a lambda as an argument, thus the example is flawed. Removed the argument completely as this will default to ascending sort.
@srowen
Copy link
Member

srowen commented Jan 7, 2016

@udoklein can you survey all examples and put any changes you find into one PR?
It looks like sortByKey does take a lambda keyfunc?

    def sortByKey(self, ascending=True, numPartitions=None, keyfunc=lambda x: x):

@udoklein
Copy link
Contributor Author

udoklein commented Jan 7, 2016

Good point. The documentation did not mention the keyfunc. However it is somewhat pointless to add the default as an argument. In particular if the default is lambda x: x. Conclusion: the documentation should be fixed instead. (http://spark.apache.org/docs/latest/programming-guide.html) Where would I find the sources for the programming guide?

@srowen
Copy link
Member

srowen commented Jan 7, 2016

Look for docs/programming-guide.md

@srowen
Copy link
Member

srowen commented Jan 10, 2016

@udoklein if you want to update the docs that's fine too -- let me know if you intend to, or want o merge as is

@udoklein
Copy link
Contributor Author

Well, in order to fix the documentation I would need to understand how to properly document it such that it is also valid for other languages (Java / Scala). Since I am a beginner I do not. I double checked that the example was definitely bogus. If the function is replaced by lambda x: -x it does not change the sort order. I suggest to just merge as is.

@srowen
Copy link
Member

srowen commented Jan 10, 2016

It's not an issue in other languages since the other APIs don't take a function in sortByKey. I'm actually not sure why it exists here. The example isn't bogus right, just redundant? reversing the sign of the keys would definitely change the order right? or are you saying the method doesn't work?

@udoklein
Copy link
Contributor Author

As I said, changing the sign does NOT change the sort order. The example is bogus. The lambda would have to be either the third argument or it has to be passed as a named argument. The way it is passed passes it into the first argument.

@srowen
Copy link
Member

srowen commented Jan 10, 2016

Ah, that's the point. Yes, needs a fix for sure.

@SparkQA
Copy link

SparkQA commented Jan 10, 2016

Test build #2359 has finished for PR 10640 at commit 1282559.

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

asfgit pushed a commit that referenced this pull request Jan 11, 2016
According to the documentation the sortByKey method does not take a lambda as an argument, thus the example is flawed. Removed the argument completely as this will default to ascending sort.

Author: Udo Klein <[email protected]>

Closes #10640 from udoklein/patch-1.

(cherry picked from commit bd723bd)
Signed-off-by: Sean Owen <[email protected]>
@srowen
Copy link
Member

srowen commented Jan 11, 2016

Merged to master/1.6/1.5

asfgit pushed a commit that referenced this pull request Jan 11, 2016
According to the documentation the sortByKey method does not take a lambda as an argument, thus the example is flawed. Removed the argument completely as this will default to ascending sort.

Author: Udo Klein <[email protected]>

Closes #10640 from udoklein/patch-1.

(cherry picked from commit bd723bd)
Signed-off-by: Sean Owen <[email protected]>
@asfgit asfgit closed this in bd723bd Jan 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants