Skip to content

Commit ff20e53

Browse files
koperagenkoperagen
authored
Merge pull request #542 from Kotlin/fix-toDataFrame-valueclass
Fix toDataFrame for value classes
2 parents 03357b1 + 7ac540a commit ff20e53

File tree

4 files changed

+204
-2
lines changed
  • core

4 files changed

+204
-2
lines changed

‎core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,16 @@ import org.jetbrains.kotlinx.dataframe.impl.getListType
2121
import org.jetbrains.kotlinx.dataframe.impl.projectUpTo
2222
import org.jetbrains.kotlinx.dataframe.impl.schema.getPropertiesOrder
2323
import java.lang.reflect.InvocationTargetException
24+
import java.lang.reflect.Method
2425
import java.time.temporal.Temporal
2526
import kotlin.reflect.KClass
2627
import kotlin.reflect.KProperty
2728
import kotlin.reflect.KVisibility
2829
import kotlin.reflect.full.isSubclassOf
2930
import kotlin.reflect.full.memberProperties
31+
import kotlin.reflect.full.primaryConstructor
3032
import kotlin.reflect.full.withNullability
33+
import kotlin.reflect.jvm.isAccessible
3134
import kotlin.reflect.jvm.javaField
3235
import kotlin.reflect.typeOf
3336

@@ -141,7 +144,26 @@ internal fun convertToDataFrame(
141144
val property = it
142145
if (excludes.contains(property)) return@mapNotNull null
143146

147+
class ValueClassConverter(val unbox: Method, val box: Method)
148+
149+
val valueClassConverter = (it.returnType.classifier as? KClass<*>)?.let { kClass ->
150+
if (!kClass.isValue) null else {
151+
val constructor =
152+
requireNotNull(kClass.primaryConstructor) { "value class $kClass is expected to have primary constructor, but couldn't obtain it" }
153+
val parameter = constructor.parameters.singleOrNull()
154+
?: error("conversion of value class $kClass with multiple parameters in constructor is not yet supported")
155+
// there's no need to unwrap if underlying field is nullable
156+
if (parameter.type.isMarkedNullable) return@let null
157+
// box and unbox impl methods are part of binary API of value classes
158+
// https://youtrack.jetbrains.com/issue/KT-50518/Boxing-Unboxing-methods-for-JvmInline-value-classes-should-be-public-accessible
159+
val unbox = kClass.java.getMethod("unbox-impl")
160+
val box = kClass.java.methods.single { it.name == "box-impl" }
161+
val valueClassConverter = ValueClassConverter(unbox, box)
162+
valueClassConverter
163+
}
164+
}
144165
property.javaField?.isAccessible = true
166+
property.isAccessible = true
145167

146168
var nullable = false
147169
var hasExceptions = false
@@ -151,7 +173,19 @@ internal fun convertToDataFrame(
151173
null
152174
} else {
153175
val value = try {
154-
it.call(obj)
176+
val value = it.call(obj)
177+
/**
178+
* here we do what compiler does
179+
* @see org.jetbrains.kotlinx.dataframe.api.CreateDataFrameTests.testKPropertyGetLibrary
180+
*/
181+
if (valueClassConverter != null) {
182+
val var1 = value?.let {
183+
valueClassConverter.unbox.invoke(it)
184+
}
185+
var1?.let { valueClassConverter.box.invoke(null, var1) }
186+
} else {
187+
value
188+
}
155189
} catch (e: InvocationTargetException) {
156190
hasExceptions = true
157191
e.targetException

‎core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import org.jetbrains.kotlinx.dataframe.kind
1010
import org.jetbrains.kotlinx.dataframe.type
1111
import org.junit.Ignore
1212
import org.junit.Test
13+
import kotlin.reflect.KProperty
1314
import kotlin.reflect.typeOf
1415

1516
class CreateDataFrameTests {
@@ -224,4 +225,70 @@ class CreateDataFrameTests {
224225
println()
225226
}
226227
}
228+
229+
// nullable field here - no generated unwrapping code
230+
@JvmInline
231+
internal value class Speed(val kmh: Number?)
232+
233+
internal class PathSegment(
234+
val id: String,
235+
val speedLimit: Speed? = null,
236+
)
237+
238+
@Test
239+
fun valueClassNullableField() {
240+
val segments = listOf(PathSegment("foo", Speed(2.3)), PathSegment("bar"))
241+
242+
val df = segments.toDataFrame()
243+
df["speedLimit"].values() shouldBe listOf(Speed(2.3), null)
244+
}
245+
246+
@Test
247+
fun valueClassNullableField1() {
248+
val segments = listOf(PathSegment("foo", Speed(2.3)), PathSegment("bar", Speed(null)))
249+
250+
val df = segments.toDataFrame()
251+
df["speedLimit"].values() shouldBe listOf(Speed(2.3), Speed(null))
252+
}
253+
254+
@JvmInline
255+
internal value class Speed1(val kmh: Number)
256+
257+
internal class PathSegment1(
258+
val id: String,
259+
val speedLimit: Speed1? = null,
260+
)
261+
262+
@Test
263+
fun valueClass() {
264+
val segments = listOf(PathSegment1("foo", Speed1(2.3)), PathSegment1("bar"))
265+
266+
val df = segments.toDataFrame()
267+
df["speedLimit"].values() shouldBe listOf(Speed1(2.3), null)
268+
}
269+
270+
@Test
271+
fun testKPropertyGet() {
272+
val segment = PathSegment("bar")
273+
val result = PathSegment::speedLimit.call(segment)
274+
result shouldBe null
275+
}
276+
277+
fun call(kProperty0: KProperty<*>, obj: Any) = kProperty0.call(obj)
278+
279+
@Test
280+
fun testKPropertyCallLibrary() {
281+
val segment = PathSegment1("bar")
282+
val result = call(PathSegment1::speedLimit, segment)
283+
// Sudden result! I cannot create this value, so toString.
284+
// In the test above you can see decompiled code that "fixes" this strange wrapping
285+
result.toString() shouldBe "Speed1(kmh=null)"
286+
}
287+
288+
private class PrivateClass(val a: Int)
289+
290+
@Test
291+
fun `convert private class with public members`() {
292+
listOf(PrivateClass(1)).toDataFrame() shouldBe dataFrameOf("a")(1)
293+
}
227294
}

‎core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,16 @@ import org.jetbrains.kotlinx.dataframe.impl.getListType
2121
import org.jetbrains.kotlinx.dataframe.impl.projectUpTo
2222
import org.jetbrains.kotlinx.dataframe.impl.schema.getPropertiesOrder
2323
import java.lang.reflect.InvocationTargetException
24+
import java.lang.reflect.Method
2425
import java.time.temporal.Temporal
2526
import kotlin.reflect.KClass
2627
import kotlin.reflect.KProperty
2728
import kotlin.reflect.KVisibility
2829
import kotlin.reflect.full.isSubclassOf
2930
import kotlin.reflect.full.memberProperties
31+
import kotlin.reflect.full.primaryConstructor
3032
import kotlin.reflect.full.withNullability
33+
import kotlin.reflect.jvm.isAccessible
3134
import kotlin.reflect.jvm.javaField
3235
import kotlin.reflect.typeOf
3336

@@ -141,7 +144,26 @@ internal fun convertToDataFrame(
141144
val property = it
142145
if (excludes.contains(property)) return@mapNotNull null
143146

147+
class ValueClassConverter(val unbox: Method, val box: Method)
148+
149+
val valueClassConverter = (it.returnType.classifier as? KClass<*>)?.let { kClass ->
150+
if (!kClass.isValue) null else {
151+
val constructor =
152+
requireNotNull(kClass.primaryConstructor) { "value class $kClass is expected to have primary constructor, but couldn't obtain it" }
153+
val parameter = constructor.parameters.singleOrNull()
154+
?: error("conversion of value class $kClass with multiple parameters in constructor is not yet supported")
155+
// there's no need to unwrap if underlying field is nullable
156+
if (parameter.type.isMarkedNullable) return@let null
157+
// box and unbox impl methods are part of binary API of value classes
158+
// https://youtrack.jetbrains.com/issue/KT-50518/Boxing-Unboxing-methods-for-JvmInline-value-classes-should-be-public-accessible
159+
val unbox = kClass.java.getMethod("unbox-impl")
160+
val box = kClass.java.methods.single { it.name == "box-impl" }
161+
val valueClassConverter = ValueClassConverter(unbox, box)
162+
valueClassConverter
163+
}
164+
}
144165
property.javaField?.isAccessible = true
166+
property.isAccessible = true
145167

146168
var nullable = false
147169
var hasExceptions = false
@@ -151,7 +173,19 @@ internal fun convertToDataFrame(
151173
null
152174
} else {
153175
val value = try {
154-
it.call(obj)
176+
val value = it.call(obj)
177+
/**
178+
* here we do what compiler does
179+
* @see org.jetbrains.kotlinx.dataframe.api.CreateDataFrameTests.testKPropertyGetLibrary
180+
*/
181+
if (valueClassConverter != null) {
182+
val var1 = value?.let {
183+
valueClassConverter.unbox.invoke(it)
184+
}
185+
var1?.let { valueClassConverter.box.invoke(null, var1) }
186+
} else {
187+
value
188+
}
155189
} catch (e: InvocationTargetException) {
156190
hasExceptions = true
157191
e.targetException

‎core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import org.jetbrains.kotlinx.dataframe.kind
1010
import org.jetbrains.kotlinx.dataframe.type
1111
import org.junit.Ignore
1212
import org.junit.Test
13+
import kotlin.reflect.KProperty
1314
import kotlin.reflect.typeOf
1415

1516
class CreateDataFrameTests {
@@ -224,4 +225,70 @@ class CreateDataFrameTests {
224225
println()
225226
}
226227
}
228+
229+
// nullable field here - no generated unwrapping code
230+
@JvmInline
231+
internal value class Speed(val kmh: Number?)
232+
233+
internal class PathSegment(
234+
val id: String,
235+
val speedLimit: Speed? = null,
236+
)
237+
238+
@Test
239+
fun valueClassNullableField() {
240+
val segments = listOf(PathSegment("foo", Speed(2.3)), PathSegment("bar"))
241+
242+
val df = segments.toDataFrame()
243+
df["speedLimit"].values() shouldBe listOf(Speed(2.3), null)
244+
}
245+
246+
@Test
247+
fun valueClassNullableField1() {
248+
val segments = listOf(PathSegment("foo", Speed(2.3)), PathSegment("bar", Speed(null)))
249+
250+
val df = segments.toDataFrame()
251+
df["speedLimit"].values() shouldBe listOf(Speed(2.3), Speed(null))
252+
}
253+
254+
@JvmInline
255+
internal value class Speed1(val kmh: Number)
256+
257+
internal class PathSegment1(
258+
val id: String,
259+
val speedLimit: Speed1? = null,
260+
)
261+
262+
@Test
263+
fun valueClass() {
264+
val segments = listOf(PathSegment1("foo", Speed1(2.3)), PathSegment1("bar"))
265+
266+
val df = segments.toDataFrame()
267+
df["speedLimit"].values() shouldBe listOf(Speed1(2.3), null)
268+
}
269+
270+
@Test
271+
fun testKPropertyGet() {
272+
val segment = PathSegment("bar")
273+
val result = PathSegment::speedLimit.call(segment)
274+
result shouldBe null
275+
}
276+
277+
fun call(kProperty0: KProperty<*>, obj: Any) = kProperty0.call(obj)
278+
279+
@Test
280+
fun testKPropertyCallLibrary() {
281+
val segment = PathSegment1("bar")
282+
val result = call(PathSegment1::speedLimit, segment)
283+
// Sudden result! I cannot create this value, so toString.
284+
// In the test above you can see decompiled code that "fixes" this strange wrapping
285+
result.toString() shouldBe "Speed1(kmh=null)"
286+
}
287+
288+
private class PrivateClass(val a: Int)
289+
290+
@Test
291+
fun `convert private class with public members`() {
292+
listOf(PrivateClass(1)).toDataFrame() shouldBe dataFrameOf("a")(1)
293+
}
227294
}

0 commit comments

Comments
 (0)