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

[Compiler plugin] Propagate nullability in toDataFrame tree conversion #942

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

koperagen
Copy link
Collaborator

@koperagen koperagen commented Nov 6, 2024

There are 3 cases where plugin assumed non-null type for column and it actually should be nullable. With this fix, compile schema matches runtime schema

@koperagen koperagen added bug Something isn't working Compiler plugin Anything related to the DataFrame Compiler Plugin labels Nov 6, 2024
@koperagen koperagen added this to the 0.15.0 milestone Nov 6, 2024
@koperagen koperagen self-assigned this Nov 6, 2024
@koperagen koperagen requested a review from Jolanrensen November 6, 2024 15:45
Copy link
Collaborator

@Jolanrensen Jolanrensen left a comment

Choose a reason for hiding this comment

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

Looks good :)

@Jolanrensen
Copy link
Collaborator

Jolanrensen commented Nov 7, 2024

I do have one potential bug: How does the compiler plugin receive type T from toDataFrame? Does it actually get all the type information like when calling the function or does it assume that the type can be received from the first index of generic types?

In other words, how does it handle something like this?

import org.jetbrains.kotlinx.dataframe.*
import org.jetbrains.kotlinx.dataframe.annotations.*
import org.jetbrains.kotlinx.dataframe.api.*
import org.jetbrains.kotlinx.dataframe.io.*

@DataSchema
data class D(
    val s: String
)

class Subtree(
    val p: Int,
    val l: List<Int>,
    val ld: List<D>,
)

class Root(val a: Subtree)

class MyList(val l: List<Root?>): List<Root?> by l

fun box(): String {
    val l = listOf(
        Root(Subtree(123, listOf(1), listOf(D("ff")))),
        null
    )
    val df = MyList(l).toDataFrame(maxDepth = 2)
    df.compareSchemas(strict = true)
    return "OK"
}

@koperagen
Copy link
Collaborator Author

This is an interesting question, i updated PR with the fix

@koperagen koperagen requested a review from Jolanrensen November 7, 2024 17:30
…rame() _call_ instead of one from the return type of receiver *iterable*.toDataFrame()
@koperagen koperagen force-pushed the toDataFrame-nullable branch from be0be0a to a49bda7 Compare November 8, 2024 12:36
@koperagen koperagen merged commit c45a097 into master Nov 8, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Compiler plugin Anything related to the DataFrame Compiler Plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants