-
Notifications
You must be signed in to change notification settings - Fork 73
Move/Insert After fix #1092
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
Move/Insert After fix #1092
Conversation
fun box(): String { | ||
val df = dataFrameOf("a", "b", "c")(1, 2, 3, 4, 5, 6) | ||
val grouped = df.group { a and b }.into("g") | ||
grouped.move { g.a }.after { g.b } |
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.
unused line
@@ -527,6 +527,7 @@ class DataFrameTreeTests : BaseTest() { | |||
@Test | |||
fun moveAfter() { | |||
val moved = typed2.move { age }.after { nameAndCity.name } | |||
println(moved) |
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.
unused line i think
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.
Don't forget to set milestone, assignee and labels for generated change log
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.
Good job :) just have some questions
sourceSegments.indices.all { targetSegments[it] == sourceSegments[it] } | ||
) { | ||
throw IllegalArgumentException( | ||
"Cannot move column '${sourcePath.joinToString()}' after its child column '${targetPath.joinToString()}'", |
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.
Good catch, but may be clearer if you add something like "cannot move this column into itself" or something. Took me a while to realize why you cannot move something after a child XD
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.
maybe "its own child"? is it a proper english? makes it clearer?
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.
btw, i wonder what will happen if you df.move { a }.under { a.b }
. Same exception as with "after"?
val parent = if (parentPath.isEmpty()) df else df.getColumnGroup(parentPath) | ||
val index = parent.getColumnIndex(it.last()) | ||
val col = df.getColumn(index) | ||
val path = it.toList() |
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.
can be cast to List as well
val path = it.toList() | ||
|
||
// Find the group reference (if any) and adjust the path | ||
val groupRefIndex = path.indexOfFirst { it.isEmpty() } |
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.
What is happening here? How does the first empty path piece correspond to a group node? And at which indices can this empty path piece occur?
|
||
val columnName = if (groupRefIndex >= 0) { | ||
// Use the next segment after group reference, or previous if no next segment | ||
path.getOrNull(groupRefIndex + 1) ?: path[groupRefIndex - 1] |
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.
can this break if groupRefIndex == 0?
fun box(): String { | ||
val df = dataFrameOf("a", "b", "c")(1, 2, 3, 4, 5, 6) | ||
|
||
val dfWithD = df.insert("d") { b * c }.after { a } |
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.
maybe lint these as well? XD
insert after
plugin support