-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sortBy(ColumnReference) accept pathOf without extra cast #779
Conversation
@@ -67,4 +72,27 @@ class SortDataColumn { | |||
col.sortWith { df1, df2 -> df1[a] - df2[a] } shouldBe sortedCol | |||
col.sortWith(compareBy { it[a] }) shouldBe sortedCol | |||
} | |||
|
|||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool new dataset for unit tests! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to validate on that some high-level integration tests with more complex logic instead of low-level A.a.b
|
||
@Suppress("unused") | ||
@DataSchema | ||
interface DsSalaries { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add somewhere the link to the initial source of dataset (for example here, on that data schema), the origin, the license, the possible transformation which we did
It's a gentleman set of actions for copying of external dataset to your codebase. If we missed it earlier, let's start from that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a link. It was downloaded as is, without modifications and is public domain. So, do we need to add anything else here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved in advance, but please add minor information about dataset
ec4ec4f
to
ee753e0
Compare
Generated sources will be updated after merging this PR. |
Before this change only
df.sortBy(pathOf().cast())
would work.df.sortByDesc(pathOf().cast())
also didn't! Was very hard to figure out correct way to call these functions. PR solves it.I found this problem while working with this dataset in Datalore which can be found in their sample database. It's public domain and seems nice, let's add it.
https://www.kaggle.com/datasets/ruchi798/data-science-job-salaries. It shows a good use case