Skip to content

Clarify that schema of property with DataFrame<T>? type is not a FrameColumn #1925

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.

Sign up for GitHub

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
koperagen merged 1 commit into from
Jul 13, 2026
Merged

Clarify that schema of property with DataFrame<T>? type is not a FrameColumn #1925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/codeGen/MarkersExtractor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import kotlin.reflect.typeOf
internal fun KType.getFieldKind(): FieldKind =
FieldKind.of(

@Jolanrensen Jolanrensen Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FieldKind.of is a very strange factory function... Is it this way because it's used elsewhere? like in the compiler plugin? :)

@koperagen koperagen Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

indeed, it is used there

this,
isDataFrame = { jvmErasure == DataFrame::class },
isDataFrame = { jvmErasure == DataFrame::class && !isMarkedNullable },
isListToFrame = {
jvmErasure == List::class && (arguments[0].type?.jvmErasure?.hasAnnotation<DataSchema>() == true)
},
Expand Down
9 changes: 8 additions & 1 deletion core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/convertTo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,14 @@ internal fun AnyFrame.convertToImpl(

when (targetSchema.kind) {
ColumnKind.Value ->
convertedColumn ?: originalColumn.convertTo(to)
when {
convertedColumn != null -> convertedColumn

originalColumn.kind == ColumnKind.Frame && to.jvmErasure == DataFrame::class ->
originalColumn

@Jolanrensen Jolanrensen Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if you return originalColumn the kind won't have changed to Value, right?

@Jolanrensen Jolanrensen Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Plus, if it does change the kind, this might enable converting FrameColumn -> ValueColumn<AnyFrame> which is not allowed

@koperagen koperagen Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If originalColumn has no nulls in runtime then creating ValueColumn is a violation and our checks fail
So yes..
It seems to create misalignment one way or another.

@Jolanrensen Jolanrensen Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So isn't this step then gonna break regardless? It converts FrameColumn -> ValueColumn
But FrameColumn cannot contain nulls, so it will always need to create a ValueColumn<DataFrame<>> which is not allowed.
I don't think there's conversion possible from FrameColumn -> ValueColumn. Maybe the other way around but only if the nulls are turned into empty dataframes

@koperagen koperagen Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Approach that i chose now is keeping runtime column "narrower" than compile time - it's usually a valid approach. So, even if target type is DataFrame<T>? we just keep runtime FrameColumn where possible
let's say convertTo<...>.dropNulls { nullableDataFrameCol into "frameCol" } => valid frameCol
Problem is that it's turns out not only narrow, but also of a different kind ...

@Jolanrensen Jolanrensen Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think logic like that will only complicate things more... Can we try to keep the rules the same everywhere?

  • DataColumn<DataFrame<>?> must always be a ValueColumn, in compile time as well as runtime
  • DataColumn<DataFrame<>> must always be a FrameColumn, in both cases too

This will make it much easier for us and to explain to users. So if the target type is DataColumn<DataFrame<>?>, we turn it into a value column, if it'sDataColumn<DataFrame<>>, it becomes a frame column.

convertTo<...>.dropNulls { nullableDataFrameCol into "frameCol" } => valid frameCol

This will then also work correctly, right? because nullableDataFrameCol: DataFrame<>? so it must have been a ValueColumn, then after dropNulls, it automatically turns into a FrameColumn
(If this doesn't work for dropNulls yet, that should probably be fixed. It works correctly for update)

@koperagen koperagen Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

DataColumn<DataFrame<>?> must always be a ValueColumn, in compile time as well as runtime - existing check prevents that
If field is DataFrame<>? and in runtime there were no nulls, according to this logic toDataFrame (changing topic from convertTo because problem originates there) should create a ValueColumn - but we forbid that

@Jolanrensen Jolanrensen Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see now... that's indeed a problem. In that case DataColumn<DataFrame<>?> can indeed be a FrameColumn in runtime too... Alright:

  • DataColumn<DataFrame<>> must always be a FrameColumn in runtime
  • DataColumn<DataFrame<>?> is likely a ValueColumn but can also be a FrameColumn in runtime because it may not contain nulls.

I suppose from the convertToImpl this means we should treat ColumnKind a bit more loosely when dealing with nullable DataFrame types. But only for those. If we're explicit enough about this case it should be fine I think :)

@koperagen koperagen Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this matches my understanding of the situation.

I suppose from the convertToImpl this means we should treat ColumnKind a bit more loosely when dealing with nullable DataFrame types. But only for those. If we're explicit enough about this case it should be fine I think :)

Yeah, this should be resolved.
I see situation like this: we had a bug that used to hide a few design problems. Now the bugs is fixed, but design problems become more apparent. Same as before having DataFrame? is error prone - before it could lead to NPE, now it leads to inconsistency if specific column selectors are used, for example, or when code for schema is generated.
So i'd say DataFrame<T>? in @DataSchema is not recommended, DataFrame<T>? value in add() { } can appear if needed but has usability issues around schema printing and codegen


else -> originalColumn.convertTo(to)
}

ColumnKind.Group -> {
val column = when {
Expand Down
31 changes: 20 additions & 11 deletions core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,18 +320,27 @@ internal fun convertToDataFrame(
val kClass = returnType.classifier as KClass<*>
val fieldKind = returnType.getFieldKind()

// property type might not be of FrameColumn kind if `AnyFrame?`, but we still have to narrow it to FrameCol
// when no actual nulls are met
val shouldCreateFrameCol = kClass == DataFrame::class && !nullable

val keepSubtree =
maxDepth <= 0 && !fieldKind.shouldBeConvertedToFrameColumn && !fieldKind.shouldBeConvertedToColumnGroup
val shouldCreateValueCol = keepSubtree ||
kClass in preserveClasses ||
property in preserveProperties ||
(
!kClass.canBeUnfolded &&
!fieldKind.shouldBeConvertedToFrameColumn &&
!fieldKind.shouldBeConvertedToColumnGroup
)
maxDepth <= 0 &&
!fieldKind.shouldBeConvertedToFrameColumn &&
!fieldKind.shouldBeConvertedToColumnGroup &&
!shouldCreateFrameCol

val shouldCreateValueCol =
keepSubtree ||
kClass in preserveClasses ||
property in preserveProperties ||
(
!kClass.canBeUnfolded &&
!fieldKind.shouldBeConvertedToFrameColumn &&
!fieldKind.shouldBeConvertedToColumnGroup &&
!shouldCreateFrameCol
)

val shouldCreateFrameCol = kClass == DataFrame::class && !nullable
val shouldCreateColumnGroup = kClass == DataRow::class

if (shouldCreateFrameCol && shouldCreateValueCol) {
Expand All @@ -358,7 +367,7 @@ internal fun convertToDataFrame(
shouldCreateColumnGroup ->
DataColumn.createColumnGroup(
name = it.columnName,
df = (values as List<AnyRow>).concat(),
df = (values as List<AnyRow?>).concat(),
)

kClass.isSubclassOf(Iterable::class) ->
Expand Down
86 changes: 52 additions & 34 deletions core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/convertTo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,21 @@ class ConvertToTests {
@DataSchema
data class DataSchemaWithAnyFrame(val dfs: AnyFrame?)

private fun locationsFrame(): DataFrame<Location?> =

@Jolanrensen Jolanrensen Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What does a DataFrame with a nullable type mean? DataFrame<Location> makes sense: you have two columns "name" and "gps". But I don't know how to interpret DataFrame<Location?>.

@Jolanrensen Jolanrensen Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should we prohibit such types? Or explicitly allow them (changing "name" and "gps" to nullable versions and fill them with nulls, like in our json reader)

@koperagen koperagen Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Iterable<T>.toDataFrame(): DataFrame<T>
In this case T just happens to be Location?
Let's say, with compiler plugin enabled this will not happen

@Jolanrensen Jolanrensen Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh, that's indeed exactly what happens, I see.

@Jolanrensen Jolanrensen Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Indeed, the compiler plugin will come up with a new type. But it might be worth to define this behavior as a rule (in docs, tests, etc).

  • DataFrame<T>: T is expected to be not nullable. If it is nullable, all value-columns will become nullable and all frame-columns can contain empty dataframes.

@koperagen koperagen Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ok, but as a different issue

listOf(
Location("Home", Gps(0.0, 0.0)),
Location("Away", null),
null,
).toDataFrame()
.alsoDebug("locations:")

private fun gpsFrame(): DataFrame<Gps?> =
listOf(
Gps(0.0, 0.0),
null,
).toDataFrame()
.alsoDebug("gps:")

@Test
fun test() {
val df1 = dataFrameOf("a")(1, 2, 3)
Expand All @@ -202,67 +217,70 @@ class ConvertToTests {
}

@Test
fun `convert df with AnyFrame to itself`() {
val locationsList = listOf(
Location("Home", Gps(0.0, 0.0)),
Location("Away", null),
null,
)
val locations = locationsList
.toDataFrame()
.alsoDebug("locations:")
fun `convert df with AnyFrame containing locations to itself`() {
val locations = locationsFrame()

val gpsList = listOf(
Gps(0.0, 0.0),
null,
)
val gps = gpsList
listOf(DataSchemaWithAnyFrame(locations))
.toDataFrame()
.alsoDebug("gps:")

val df1 = listOf(
DataSchemaWithAnyFrame(locations),
).toDataFrame()
.alsoDebug("df1:")
.convertTo<DataSchemaWithAnyFrame>()
}

df1.convertTo<DataSchemaWithAnyFrame>()
@Test
fun `convert df with AnyFrame containing gps to itself`() {
val gps = gpsFrame()

val df2 = listOf(
DataSchemaWithAnyFrame(gps),
).toDataFrame()
listOf(DataSchemaWithAnyFrame(gps))
.toDataFrame()
.alsoDebug("df2:")
.convertTo<DataSchemaWithAnyFrame>()
}

df2.convertTo<DataSchemaWithAnyFrame>()
@Test
fun `convert df with preserved AnyFrame containing null and gps to itself`() {
val gps = gpsFrame()

val df3 = listOf(
listOf(
DataSchemaWithAnyFrame(null),
DataSchemaWithAnyFrame(gps),
).toDataFrame { properties { preserve(DataFrame::class) } }
.alsoDebug("df3 before convert:")
.convertTo<DataSchemaWithAnyFrame>()
}

df3.convertTo<DataSchemaWithAnyFrame>()

val df4 = listOf(
@Test
fun `convert df with preserved null AnyFrame to itself`() {
listOf(
DataSchemaWithAnyFrame(null),
).toDataFrame { properties { preserve(DataFrame::class) } }
.alsoDebug("df4 before convert:")
.convertTo<DataSchemaWithAnyFrame>()
}

df4.convertTo<DataSchemaWithAnyFrame>()
@Test
fun `convert raw df with AnyFrame column to itself`() {
val locations = locationsFrame()
val gps = gpsFrame()

val df5a: DataFrame<*> = dataFrameOf(
val df: DataFrame<*> = dataFrameOf(
columnOf(locations, gps, null).named("dfs"),
).alsoDebug("df5a:")

df5a.convertTo<DataSchemaWithAnyFrame>()
df.convertTo<DataSchemaWithAnyFrame>()
}

@Test
fun `convert df with preserved mixed AnyFrame values to itself repeatedly`() {
val locations = locationsFrame()
val gps = gpsFrame()

val df5 = listOf(
listOf(
DataSchemaWithAnyFrame(null),
DataSchemaWithAnyFrame(locations),
DataSchemaWithAnyFrame(gps),
).toDataFrame { properties { preserve(DataFrame::class) } }
.alsoDebug("df5 before convert:")

df5.convertTo<DataSchemaWithAnyFrame>()
.convertTo<DataSchemaWithAnyFrame>()
.alsoDebug("df5 after convert:")
.convertTo<DataSchemaWithAnyFrame>()
.alsoDebug("df5 after second convert:")
Expand Down
17 changes: 17 additions & 0 deletions core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/codeGen/CodeGenerationTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import io.kotest.matchers.shouldBe
import org.jetbrains.kotlinx.dataframe.AnyRow
import org.jetbrains.kotlinx.dataframe.ColumnsScope
import org.jetbrains.kotlinx.dataframe.DataColumn
import org.jetbrains.kotlinx.dataframe.DataFrame
import org.jetbrains.kotlinx.dataframe.DataRow
import org.jetbrains.kotlinx.dataframe.annotations.DataSchema
import org.jetbrains.kotlinx.dataframe.api.columnOf
import org.jetbrains.kotlinx.dataframe.api.dataFrameOf
import org.jetbrains.kotlinx.dataframe.api.default
Expand All @@ -16,16 +18,19 @@ import org.jetbrains.kotlinx.dataframe.api.groupBy
import org.jetbrains.kotlinx.dataframe.api.into
import org.jetbrains.kotlinx.dataframe.api.move
import org.jetbrains.kotlinx.dataframe.api.pathOf
import org.jetbrains.kotlinx.dataframe.api.print
import org.jetbrains.kotlinx.dataframe.api.schema
import org.jetbrains.kotlinx.dataframe.api.toCodeString
import org.jetbrains.kotlinx.dataframe.api.under
import org.jetbrains.kotlinx.dataframe.columns.ColumnGroup
import org.jetbrains.kotlinx.dataframe.impl.codeGen.ReplCodeGenerator
import org.jetbrains.kotlinx.dataframe.impl.codeGen.ReplCodeGeneratorImpl
import org.jetbrains.kotlinx.dataframe.impl.toCamelCaseByDelimiters
import org.jetbrains.kotlinx.dataframe.schema.ColumnSchema
import org.jetbrains.kotlinx.dataframe.testSets.person.BaseTest
import org.jetbrains.kotlinx.dataframe.testSets.person.Person
import org.junit.Test
import kotlin.reflect.typeOf
import kotlin.test.assertEquals

class CodeGenerationTests : BaseTest() {
Expand Down Expand Up @@ -570,6 +575,18 @@ class CodeGenerationTests : BaseTest() {
assertEquals(expected, df.generateDataClasses().value)
}

@DataSchema
class C(val i: Int)

@DataSchema
class Schema(val df: DataFrame<C>?)

@Test
fun extractNullableDataFrameSchema() {
val schema = MarkersExtractor.get<Schema>().schema
schema.columns["df"] shouldBe ColumnSchema.Value(typeOf<DataFrame<C>?>())
}

// region Tests for generateX functions

@Test
Expand Down
Loading