Skip to content

Map unfold #1097

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

Merged
merged 4 commits into from
Mar 17, 2025
Merged

Map unfold #1097

merged 4 commits into from
Mar 17, 2025

Conversation

AndreiKingsley
Copy link
Collaborator

@AndreiKingsley AndreiKingsley commented Mar 14, 2025

Fixes #896.

Now Map is treated like value class and not unfolded. Also fix unfolding behavior in unfold (now it's the same as in toDataFrame())

@@ -69,6 +76,7 @@ internal val KClass<*>.isValueType: Boolean
this in valueTypes ||
this.isSubclassOf(Number::class) ||
this.isSubclassOf(Enum::class) ||
this.isSubclassOf(Map::class) ||
Copy link
Collaborator

@koperagen koperagen Mar 14, 2025

Choose a reason for hiding this comment

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

This is not consistent with plugin - here it's all subclasses, in plugin only Map and MutableMap specifically. Which one should it be?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would do it like the plugin. There are many classes that have List or Map as supertype but they should probably be treated like a normal class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only Map and MutableMap. Fixed.

@Jolanrensen Jolanrensen self-requested a review March 14, 2025 12:42
@@ -58,6 +58,13 @@ private val valueTypes = setOf(
kotlinx.datetime.DateTimeUnit::class,
)

/**
* Check
Copy link
Collaborator

Choose a reason for hiding this comment

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

check what? ;P

)

/**
* Check
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

very clean solution :)

@AndreiKingsley AndreiKingsley merged commit 5091e23 into master Mar 17, 2025
3 checks passed
@AndreiKingsley AndreiKingsley deleted the map_unfold branch March 17, 2025 09:37
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.

unfold / toDataFrame behavior for Maps
3 participants