-
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
Fixed rename behavior #419
Conversation
…med at once. added issue link for replace.with fixed renameToCamelCase using new replace behavior
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 left minor comments for generated docs instead of the real sources
core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/rename.kt
Outdated
Show resolved
Hide resolved
core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/rename.kt
Outdated
Show resolved
Hide resolved
core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/rename.kt
Outdated
Show resolved
Hide resolved
|
||
// perform rename in nodes | ||
tree.allChildrenNotNull().forEach { node -> | ||
val column = selectedColumns.find { it.data == node.data } ?: return@forEach |
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's data here? From collectTree definition it seems its a DataColumn, and in this case equals will iterate over elements of the column in worst case. Quite a heavy operation, although probably it shouldn't happen in this algorithm. Can you double check and maybe provide a comment with justification?
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.
is it possible that there's two identical columns under different column groups and second one will never be renamed?
edit. looks like no, because you iterate over all tree nodes
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.
data
is a column with no path anymore (collectTree()
removes it and ColumnWithPath.data yields the original column). So to get that path back I need to find it in selectedColumns
(I'll change this to a Map actually). I would like to use node.pathFromRoot()
but unfortunately, the tree might already have had some node renamed, so this would generate wrong results.
|
||
// build up a new DataFrame using the modified names | ||
var newDf = DataFrame.empty(df.rowsCount()).cast<T>() | ||
tree.allChildrenNotNull().forEach { node -> |
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.
This tree structure is used in other places to create dataframes? Is there an existing code to re-create a df from it? Approach with insert under looks suspicious because it's a "mutation" of an immutable data structure in a loop
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.
There doesn't appear to be such a thing. Usually dataframes are built from the ground up with columns recursively. I'll try to create a sort-of mapping function for TreeNode with which I can create a new dataframe with columns I create on the fly
…iews and using new map function
Feel free to fix or merge this branch if it's done when I'm away :) |
Fixes #405
Column groups and their children can now be renamed in the same call using a rewritten method.
renameToCamelCase
was also updated with this new method (no longer does it need 2 or 3 runs over the dataframe) and I actually moved the the fix for #302 totoCamelCase()
, since it saves a run over the DF and makes the calls more consistent.