Skip to content

Commit

Permalink
Merge pull request #467 from Kotlin/better-rendering
Browse files Browse the repository at this point in the history 
Improved rendering of types with arguments, like `Comparable<*>`
  • Loading branch information
@Jolanrensen
Jolanrensen authored Oct 16, 2023
2 parents 835de4b + 4646f6a commit 71ab27a
Show file tree
Hide file tree
Showing 147 changed files with 2,159 additions and 1,795 deletions.
4 changes: 2 additions & 2 deletions core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/filter.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import org.jetbrains.kotlinx.dataframe.DataFrame
import org.jetbrains.kotlinx.dataframe.Predicate
import org.jetbrains.kotlinx.dataframe.RowFilter
import org.jetbrains.kotlinx.dataframe.columns.ColumnReference
import org.jetbrains.kotlinx.dataframe.impl.toIndices
import org.jetbrains.kotlinx.dataframe.impl.getTrueIndices
import org.jetbrains.kotlinx.dataframe.indices
import kotlin.reflect.KProperty

Expand All @@ -27,7 +27,7 @@ public fun <T> DataFrame<T>.filter(predicate: RowFilter<T>): DataFrame<T> =
}.let { get(it) }

public fun <T> DataFrame<T>.filterBy(column: ColumnSelector<T, Boolean>): DataFrame<T> =
getRows(getColumn(column).toList().toIndices())
getRows(getColumn(column).toList().getTrueIndices())

public fun <T> DataFrame<T>.filterBy(column: String): DataFrame<T> = filterBy { column.toColumnOf() }

Expand Down
50 changes: 36 additions & 14 deletions core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Rendering.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,26 +48,48 @@ internal fun renderType(column: ColumnSchema) =
else -> throw NotImplementedError()
}

internal val classesToBeRenderedShort = setOf(URL::class, LocalDateTime::class, LocalTime::class)
internal fun renderType(type: KType?): String {
return when (type?.classifier) {
null -> "*"

internal fun renderType(type: KType): String =
when (type.classifier) {
Nothing::class -> "Nothing" + if (type.isMarkedNullable) "?" else ""
List::class -> {
val argument = type.arguments[0].type?.let { renderType(it) } ?: "*"
"List<$argument>"
}

else -> {
val result = type.toString()
if (classesToBeRenderedShort.contains(type.classifier) ||
result.startsWith("kotlin.") ||
result.startsWith("org.jetbrains.kotlinx.dataframe.")
) {
(type.jvmErasure.simpleName?.let { if (type.isMarkedNullable) "$it?" else it }) ?: result
} else result
val fullName = type.jvmErasure.qualifiedName ?: return type.toString()
val name = when {
type.classifier == URL::class ->
fullName.removePrefix("java.net.")

type.classifier in listOf(LocalDateTime::class, LocalTime::class) ->
fullName.removePrefix("java.time.")

fullName.startsWith("kotlin.collections.") ->
fullName.removePrefix("kotlin.collections.")

fullName.startsWith("kotlin.") ->
fullName.removePrefix("kotlin.")

fullName.startsWith("org.jetbrains.kotlinx.dataframe.") ->
fullName.removePrefix("org.jetbrains.kotlinx.dataframe.")

else -> fullName
}

buildString {
append(name)
if (type.arguments.isNotEmpty()) {
val arguments = type.arguments.joinToString {
renderType(it.type)
}
append("<$arguments>")
}
if (type.isMarkedNullable) {
append("?")
}
}
}
}
}

internal fun renderType(column: AnyCol) =
when (column.kind()) {
Expand Down
99 changes: 69 additions & 30 deletions core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/TypeUtils.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import kotlin.reflect.KClass
import kotlin.reflect.KType
import kotlin.reflect.KTypeParameter
import kotlin.reflect.KTypeProjection
import kotlin.reflect.KVariance
import kotlin.reflect.KVariance.*
import kotlin.reflect.KVisibility
import kotlin.reflect.full.allSuperclasses
import kotlin.reflect.full.createType
Expand Down Expand Up @@ -50,37 +52,47 @@ internal fun KType.projectUpTo(superClass: KClass<*>): KType {
}

/**
* Changes generic type parameters to `Any?`, like `List<T> -> List<Any?>`.
* Changes generic type parameters to its upperbound, like `List<T> -> List<Any?>` and
* `Comparable<T> -> Comparable<Nothing>`.
* Works recursively as well.
*/
@PublishedApi
internal fun KType.eraseGenericTypeParameters(): KType {
fun KType.eraseRecursively(): Pair<Boolean, KType> {
var replaced = false
val arguments = arguments.map {
val type = it.type
val (replacedDownwards, newType) = when {
type == null -> typeOf<Any?>()

type.classifier is KTypeParameter -> {
replaced = true
(type.classifier as KTypeParameter).upperBounds.firstOrNull() ?: typeOf<Any?>()
internal fun KType.replaceGenericTypeParametersWithUpperbound(): KType {
fun KType.replaceRecursively(): Pair<Boolean, KType> {
var replacedAnyArgument = false
val arguments = arguments.mapIndexed { i, it ->
val oldType = it.type ?: return@mapIndexed it // is * if null, we'll leave those be

val type = when {
oldType.classifier is KTypeParameter -> { // e.g. T
replacedAnyArgument = true

// resolve the variance (`in`/`out`/``) of the argument in the original class
val varianceInClass = (classifier as? KClass<*>)?.typeParameters?.getOrNull(i)?.variance
when (varianceInClass) {
INVARIANT, OUT, null ->
(oldType.classifier as KTypeParameter).upperBounds.firstOrNull() ?: typeOf<Any?>()

// Type<in T> cannot be replaced with Type<Any?>, instead it should be replaced with Type<Nothing>
// TODO: issue #471
IN -> nothingType(false)
}
}

else -> type
}.eraseRecursively()

if (replacedDownwards) replaced = true
else -> oldType
}
val (replacedDownwards, newType) = type.replaceRecursively()
if (replacedDownwards) replacedAnyArgument = true

KTypeProjection.invariant(newType)
}
return Pair(
first = replaced,
second = if (replaced) jvmErasure.createType(arguments, isMarkedNullable) else this,
first = replacedAnyArgument,
second = if (replacedAnyArgument) jvmErasure.createType(arguments, isMarkedNullable) else this,
)
}

return eraseRecursively().second
return replaceRecursively().second
}

internal fun inheritanceChain(subClass: KClass<*>, superClass: KClass<*>): List<Pair<KClass<*>, KType>> {
Expand Down Expand Up @@ -221,12 +233,16 @@ internal fun commonParents(classes: Iterable<KClass<*>>): List<KClass<*>> =
* @receiver the types to find the common type for
* @return the common type including listify behaviour
*
* @see commonType
* @param useStar if true, `*` will be used to fill in generic type parameters instead of `Any?`
* (for invariant/out variance) or `Nothing` (for in variance)
*
* @see Iterable.commonType
*/
internal fun Iterable<KType>.commonTypeListifyValues(): KType {
internal fun Iterable<KType>.commonTypeListifyValues(useStar: Boolean = true): KType {
val distinct = distinct()
val nullable = distinct.any { it.isMarkedNullable }
return when {
distinct.isEmpty() -> Any::class.createStarProjectedType(distinct.any { it.isMarkedNullable })
distinct.isEmpty() -> typeOf<Any>().withNullability(nullable)
distinct.size == 1 -> distinct.single()
else -> {
val classes = distinct.map {
Expand Down Expand Up @@ -269,20 +285,43 @@ internal fun Iterable<KType>.commonTypeListifyValues(): KType {
}

else -> {
val kclass = commonParent(distinct.map { it.jvmErasure }) ?: return typeOf<Any>()
val projections = distinct.map { it.projectUpTo(kclass).eraseGenericTypeParameters() }
require(projections.all { it.jvmErasure == kclass })
val arguments = List(kclass.typeParameters.size) { i ->
val kClass = commonParent(distinct.map { it.jvmErasure })
?: return typeOf<Any>().withNullability(nullable)
val projections = distinct
.map { it.projectUpTo(kClass).replaceGenericTypeParametersWithUpperbound() }
require(projections.all { it.jvmErasure == kClass })

val arguments = List(kClass.typeParameters.size) { i ->
val typeParameter = kClass.typeParameters[i]
val projectionTypes = projections
.map { it.arguments[i].type }
.filterNot { it in distinct } // avoid infinite recursion

val type = projectionTypes.filterNotNull().commonTypeListifyValues()
KTypeProjection.invariant(type)
when {
projectionTypes.isEmpty() && typeParameter.variance == KVariance.IN -> {
if (useStar) {
KTypeProjection.STAR
} else {
KTypeProjection.invariant(nothingType(false))
}
}

else -> {
val commonType = projectionTypes.filterNotNull().commonTypeListifyValues(useStar)
if (commonType == typeOf<Any?>() && useStar) {
KTypeProjection.STAR
} else {
KTypeProjection(typeParameter.variance, commonType)
}
}
}
}

if (kclass == Nothing::class) nothingType(nullable = distinct.any { it.isMarkedNullable })
else kclass.createType(arguments, distinct.any { it.isMarkedNullable })
if (kClass == Nothing::class) {
nothingType(nullable)
} else {
kClass.createType(arguments, nullable)
}
}
}
}
Expand Down
57 changes: 43 additions & 14 deletions core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Utils.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import kotlin.reflect.KClass
import kotlin.reflect.KProperty
import kotlin.reflect.KType
import kotlin.reflect.KTypeProjection
import kotlin.reflect.KVariance
import kotlin.reflect.full.createType
import kotlin.reflect.full.findAnnotation
import kotlin.reflect.full.isSubtypeOf
Expand Down Expand Up @@ -67,16 +68,16 @@ internal fun <T : Any> convert(src: Int, tartypeOf: KClass<T>): T = when (tartyp
else -> throw NotImplementedError("Casting int to $tartypeOf is not supported")
}

internal fun BooleanArray.toIndices(): List<Int> {
internal fun BooleanArray.getTrueIndices(): List<Int> {
val res = ArrayList<Int>(size)
for (i in 0 until size)
for (i in indices)
if (this[i]) res.add(i)
return res
}

internal fun List<Boolean>.toIndices(): List<Int> {
internal fun List<Boolean>.getTrueIndices(): List<Int> {
val res = ArrayList<Int>(size)
for (i in 0 until size)
for (i in indices)
if (this[i]) res.add(i)
return res
}
Expand Down Expand Up @@ -136,27 +137,55 @@ internal fun Iterable<KClass<*>>.commonType(nullable: Boolean, upperBound: KType
/**
* Returns the common supertype of the given types.
*
* @see commonTypeListifyValues
* @param useStar if true, `*` will be used to fill in generic type parameters instead of `Any?`
* (for invariant/out variance) or `Nothing` (for in variance)
*
* @see Iterable.commonTypeListifyValues
*/
internal fun Iterable<KType?>.commonType(): KType {
internal fun Iterable<KType?>.commonType(useStar: Boolean = true): KType {
val distinct = distinct()
val nullable = distinct.any { it?.isMarkedNullable ?: true }
return when {
distinct.isEmpty() || distinct.contains(null) -> Any::class.createStarProjectedType(nullable)
distinct.isEmpty() || distinct.contains(null) -> typeOf<Any>().withNullability(nullable)
distinct.size == 1 -> distinct.single()!!

else -> {
val kclass = commonParent(distinct.map { it!!.jvmErasure }) ?: return typeOf<Any>()
val projections = distinct.map { it!!.projectUpTo(kclass).eraseGenericTypeParameters() }
require(projections.all { it.jvmErasure == kclass })
val arguments = List(kclass.typeParameters.size) { i ->
// common parent class of all KTypes
val kClass = commonParent(distinct.map { it!!.jvmErasure })
?: return typeOf<Any>().withNullability(nullable)

// all KTypes projected to the common parent class with filled-in generic type parameters (no <T>, but <UpperBound>)
val projections = distinct
.map { it!!.projectUpTo(kClass).replaceGenericTypeParametersWithUpperbound() }
require(projections.all { it.jvmErasure == kClass })

// make new type arguments for the common parent class
val arguments = List(kClass.typeParameters.size) { i ->
val typeParameter = kClass.typeParameters[i]
val projectionTypes = projections
.map { it.arguments[i].type }
.filterNot { it in distinct } // avoid infinite recursion

val type = projectionTypes.commonType()
KTypeProjection.invariant(type)
when {
projectionTypes.isEmpty() && typeParameter.variance == KVariance.IN -> {
if (useStar) {
KTypeProjection.STAR
} else {
KTypeProjection.invariant(nothingType(false))
}
}

else -> {
val commonType = projectionTypes.commonType(useStar)
if (commonType == typeOf<Any?>() && useStar) {
KTypeProjection.STAR
} else {
KTypeProjection(typeParameter.variance, commonType)
}
}
}
}
kclass.createType(arguments, nullable)
kClass.createType(arguments, nullable)
}
}
}
Expand Down
11 changes: 7 additions & 4 deletions ...ated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/constructors.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,15 @@ import org.jetbrains.kotlinx.dataframe.api.dataFrameOf
import org.jetbrains.kotlinx.dataframe.api.indices
import org.jetbrains.kotlinx.dataframe.api.toColumnOf
import org.jetbrains.kotlinx.dataframe.api.toDataFrame
import org.jetbrains.kotlinx.dataframe.columns.*
import org.jetbrains.kotlinx.dataframe.columns.ColumnResolutionContext
import org.jetbrains.kotlinx.dataframe.columns.ColumnSet
import org.jetbrains.kotlinx.dataframe.columns.ColumnWithPath
import org.jetbrains.kotlinx.dataframe.columns.toColumnsSetOf
import org.jetbrains.kotlinx.dataframe.impl.DataFrameReceiver
import org.jetbrains.kotlinx.dataframe.impl.DataRowImpl
import org.jetbrains.kotlinx.dataframe.impl.asList
import org.jetbrains.kotlinx.dataframe.impl.eraseGenericTypeParameters
import org.jetbrains.kotlinx.dataframe.impl.guessValueType
import org.jetbrains.kotlinx.dataframe.impl.replaceGenericTypeParametersWithUpperbound
import org.jetbrains.kotlinx.dataframe.index
import org.jetbrains.kotlinx.dataframe.nrow
import kotlin.reflect.KClass
Expand Down Expand Up @@ -58,7 +61,7 @@ internal fun <T, R> ColumnsContainer<T>.newColumn(
Infer.Nulls -> DataColumn.create(
name = name,
values = values,
type = type.withNullability(nullable).eraseGenericTypeParameters(),
type = type.withNullability(nullable).replaceGenericTypeParametersWithUpperbound(),
infer = Infer.None,
)

Expand All @@ -71,7 +74,7 @@ internal fun <T, R> ColumnsContainer<T>.newColumn(
Infer.None -> DataColumn.create(
name = name,
values = values,
type = type.eraseGenericTypeParameters(),
type = type.replaceGenericTypeParametersWithUpperbound(),
infer = Infer.None,
)
}
Expand Down
6 changes: 4 additions & 2 deletions ...ted-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/testSets/person/PivotTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class PivotTests {

data["age"].type() shouldBe typeOf<List<Int>>()
data["city"].type() shouldBe typeOf<String>()
data["weight"].type() shouldBe typeOf<Comparable<Any>>()
data["weight"].type() shouldBe typeOf<Comparable<*>>() // Comparable<String + Int> -> Comparable<Nothing | *>

res.renderToString(columnTypes = true, title = true) shouldBe
defaultExpected.group { drop(1) }.into("key").renderToString(columnTypes = true, title = true)
Expand Down Expand Up @@ -136,7 +136,9 @@ class PivotTests {

@Test
fun `pivot two values`() {
val pivoted = typed.pivot(inward = false) { key }.groupBy { name }
val pivoted = typed
.pivot(inward = false) { key }
.groupBy { name }
.values { value and (expr { value?.toString() } into "str") default "-" }

val expected = defaultExpected.replace("age", "city", "weight").with {
Expand Down
8 changes: 5 additions & 3 deletions ...ated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/types/TypeProjectionTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@ class TypeProjectionTests {

@Test
fun `common type tests`() {
listOf(typeOf<List<Int>>(), typeOf<Set<Double?>>()).commonType() shouldBe typeOf<Collection<Number?>>()
listOf(typeOf<List<Int>>(), typeOf<Set<*>>()).commonType() shouldBe typeOf<Collection<Any?>>()
listOf(typeOf<List<Int>>(), typeOf<Set<*>?>()).commonType() shouldBe typeOf<Collection<Any?>?>()
listOf(typeOf<List<Int>>(), typeOf<Set<Double?>>()).commonType() shouldBe typeOf<Collection<out Number?>>()
listOf(typeOf<List<Int>>(), typeOf<Set<*>>()).commonType(false) shouldBe typeOf<Collection<out Any?>>()
listOf(typeOf<List<Int>>(), typeOf<Set<*>>()).commonType() shouldBe typeOf<Collection<*>>()
listOf(typeOf<List<Int>>(), typeOf<Set<*>?>()).commonType(false) shouldBe typeOf<Collection<out Any?>?>()
listOf(typeOf<List<Int>>(), typeOf<Set<*>?>()).commonType() shouldBe typeOf<Collection<*>?>()
}
}
Loading

0 comments on commit 71ab27a

Please sign in to comment.