Skip to content
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

rename impl fix #1104

Merged
merged 2 commits into from
Mar 22, 2025
Merged

rename impl fix #1104

merged 2 commits into from
Mar 22, 2025

Conversation

AndreiKingsley
Copy link
Collaborator

@AndreiKingsley AndreiKingsley commented Mar 19, 2025

fixes #1067

@Jolanrensen Jolanrensen self-requested a review March 19, 2025 15:02
return renameImpl { newNames[i++] }
// associate old column names with new ones
val selectedColumns = df.getColumnsWithPaths(columns)
val oldToNew = newNames.mapIndexed { index, newName ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to "associate", there's a function named exactly that ;p maybe you can use it here too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need indices here, but there's no associateIndexed unfortunately xD.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withIndex().associate {} works, but then again, this works too :p

// associate old column names with new ones
val selectedColumns = df.getColumnsWithPaths(columns)
val oldToNew = newNames.mapIndexed { index, newName ->
selectedColumns[index].name to newName
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now throw an exception even there are fewer columns selected than names given. If we want to maintain the old behavior, we should limit newNames to the number of selected columns before mapping

selectedColumns[index].name to newName
}.toMap()

return renameImpl { column -> oldToNew[column.name] ?: column.name }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens for columns with the same name at different paths? Probably create the map by path, not by name

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point!

@AndreiKingsley AndreiKingsley merged commit 692eba8 into master Mar 22, 2025
6 checks passed
@AndreiKingsley AndreiKingsley deleted the rename_fix branch March 22, 2025 11:25
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.

rename(Pair<String, String>) assigns new names in the wrong order
2 participants