added getterLike checks for KCallable in toDataFrame DSL. reworked pr…

…operty order sorting with multiple constructors.
Kotlin · Apr 8, 2024 · 89ddd20 · 89ddd20
1 parent 1dd6151
commit 89ddd20
Show file tree

Hide file tree

Showing 10 changed files with 168 additions and 66 deletions.
diff --git a/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt b/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt
@@ -122,7 +122,7 @@ public interface TraversePropertiesDsl {
 
     /**
      * Skip given [properties] during recursive (dfs) traversal.
-     * These can also be getter-like functions.
+     * These can also be getter-like functions (like `getX()` or `isX()`).
      */
     public fun exclude(vararg properties: KCallable<*>)
 
@@ -133,7 +133,7 @@ public interface TraversePropertiesDsl {
 
     /**
      * Store given [properties] in ValueColumns without transformation into ColumnGroups or FrameColumns.
-     * These can also be getter-like functions.
+     * These can also be getter-like functions (like `getX()` or `isX()`).
      */
     public fun preserve(vararg properties: KCallable<*>)
 }

diff --git a/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Utils.kt b/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Utils.kt
@@ -17,13 +17,15 @@ import java.math.BigInteger
 import kotlin.reflect.KCallable
 import kotlin.reflect.KClass
 import kotlin.reflect.KFunction
+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
 import kotlin.reflect.full.starProjectedType
+import kotlin.reflect.full.valueParameters
 import kotlin.reflect.full.withNullability
 import kotlin.reflect.jvm.jvmErasure
 import kotlin.reflect.typeOf
@@ -338,8 +340,22 @@ internal fun List<String>.joinToCamelCaseString(): String {
         .replaceFirstChar { it.lowercaseChar() }
 }
 
+internal fun KFunction<*>.isGetterLike(): Boolean =
+    (name.startsWith("get") || name.startsWith("is")) && valueParameters.isEmpty()
+
+internal fun KCallable<*>.isGetterLike(): Boolean =
+    when (this) {
+        is KProperty<*> -> true
+        is KFunction<*> -> isGetterLike()
+        else -> false
+    }
+
 @PublishedApi
-internal val <T> KCallable<T>.columnName: String
+internal val KProperty<*>.columnName: String
+    get() = findAnnotation<ColumnName>()?.name ?: name
+
+@PublishedApi
+internal val KCallable<*>.columnName: String
     get() = findAnnotation<ColumnName>()?.name
         ?: when (this) {
             // for defining the column names based on a getter-function, we use the function name minus the get/is prefix
@@ -349,5 +365,7 @@ internal val <T> KCallable<T>.columnName: String
                     .removePrefix("is")
                     .replaceFirstChar { it.lowercase() }
 
+            is KProperty<*> -> this.columnName
+
             else -> name
         }
diff --git a/...generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt b/...generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt
@@ -18,9 +18,9 @@ import org.jetbrains.kotlinx.dataframe.impl.asList
 import org.jetbrains.kotlinx.dataframe.impl.columnName
 import org.jetbrains.kotlinx.dataframe.impl.emptyPath
 import org.jetbrains.kotlinx.dataframe.impl.getListType
+import org.jetbrains.kotlinx.dataframe.impl.isGetterLike
 import org.jetbrains.kotlinx.dataframe.impl.projectUpTo
-import org.jetbrains.kotlinx.dataframe.impl.schema.getPropertyOrderFromAnyConstructor
-import org.jetbrains.kotlinx.dataframe.impl.schema.getPropertyOrderFromPrimaryConstructor
+import org.jetbrains.kotlinx.dataframe.impl.schema.sortWithConstructors
 import java.lang.reflect.InvocationTargetException
 import java.lang.reflect.Method
 import java.time.temporal.Temporal
@@ -32,7 +32,6 @@ import kotlin.reflect.full.isSubclassOf
 import kotlin.reflect.full.memberFunctions
 import kotlin.reflect.full.memberProperties
 import kotlin.reflect.full.primaryConstructor
-import kotlin.reflect.full.valueParameters
 import kotlin.reflect.full.withNullability
 import kotlin.reflect.jvm.isAccessible
 import kotlin.reflect.jvm.javaField
@@ -93,6 +92,11 @@ internal class CreateDataFrameDslImpl<T>(
         }
 
         override fun exclude(vararg properties: KCallable<*>) {
+            for (prop in properties) {
+                require(prop.isGetterLike()) {
+                    "${prop.name} is not a property or getter-like function. Only those are traversed and can be excluded."
+                }
+            }
             excludeProperties.addAll(properties)
         }
 
@@ -105,11 +109,22 @@ internal class CreateDataFrameDslImpl<T>(
         }
 
         override fun preserve(vararg properties: KCallable<*>) {
+            for (prop in properties) {
+                require(prop.isGetterLike()) {
+                    "${prop.name} is not a property or getter-like function. Only those are traversed and can be preserved."
+                }
+            }
             preserveProperties.addAll(properties)
         }
     }
 
     override fun properties(vararg roots: KCallable<*>, maxDepth: Int, body: (TraversePropertiesDsl.() -> Unit)?) {
+        for (prop in roots) {
+            require(prop.isGetterLike()) {
+                "${prop.name} is not a property or getter-like function. Only those are traversed and can be added as roots."
+            }
+        }
+
         val dsl = configuration.clone()
         if (body != null) {
             body(dsl)
@@ -149,41 +164,20 @@ internal fun convertToDataFrame(
     preserveProperties: Set<KCallable<*>>,
     maxDepth: Int,
 ): AnyFrame {
-    val primaryConstructorOrder = getPropertyOrderFromPrimaryConstructor(clazz)
-    val anyConstructorOrder = getPropertyOrderFromAnyConstructor(clazz)
-
     val properties: List<KCallable<*>> = roots
         .ifEmpty {
             clazz.memberProperties
-                .filter { it.visibility == KVisibility.PUBLIC && it.valueParameters.isEmpty() }
+                .filter { it.visibility == KVisibility.PUBLIC }
         }
 
-        // fall back to getter functions for pojo-like classes
+        // fall back to getter functions for pojo-like classes if no member properties were found
         .ifEmpty {
             clazz.memberFunctions
-                .filter {
-                    it.visibility == KVisibility.PUBLIC &&
-                        it.valueParameters.isEmpty() &&
-                        (it.name.startsWith("get") || it.name.startsWith("is"))
-                }
+                .filter { it.visibility == KVisibility.PUBLIC && it.isGetterLike() }
         }
 
-        // sort properties by order of primary-ish constructor
-        .let {
-            val names = it.map { it.columnName }.toSet()
-
-            // use the primary constructor order if it's available,
-            // else try to find a constructor that matches all properties
-            val order = primaryConstructorOrder
-                ?: anyConstructorOrder.firstOrNull { map ->
-                    names.all { it in map.keys }
-                }
-            if (order != null) {
-                it.sortedBy { order[it.columnName] ?: Int.MAX_VALUE }
-            } else {
-                it
-            }
-        }
+        // sort properties by order in constructors
+        .sortWithConstructors(clazz)
 
     val columns = properties.mapNotNull {
         val property = it

diff --git a/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/schema/Utils.kt b/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/schema/Utils.kt
@@ -13,10 +13,13 @@ import org.jetbrains.kotlinx.dataframe.columns.ColumnKind
 import org.jetbrains.kotlinx.dataframe.columns.FrameColumn
 import org.jetbrains.kotlinx.dataframe.columns.ValueColumn
 import org.jetbrains.kotlinx.dataframe.hasNulls
+import org.jetbrains.kotlinx.dataframe.impl.columnName
 import org.jetbrains.kotlinx.dataframe.impl.commonType
+import org.jetbrains.kotlinx.dataframe.impl.isGetterLike
 import org.jetbrains.kotlinx.dataframe.schema.ColumnSchema
 import org.jetbrains.kotlinx.dataframe.schema.DataFrameSchema
 import org.jetbrains.kotlinx.dataframe.type
+import kotlin.reflect.KCallable
 import kotlin.reflect.KClass
 import kotlin.reflect.full.primaryConstructor
 import kotlin.reflect.full.withNullability
@@ -148,11 +151,46 @@ internal fun getPropertyOrderFromPrimaryConstructor(clazz: KClass<*>): Map<Strin
         ?.mapIndexed { i, v -> v to i }
         ?.toMap()
 
-internal fun getPropertyOrderFromAnyConstructor(clazz: KClass<*>): List<Map<String, Int>> =
+internal fun getPropertyOrderFromAllConstructors(clazz: KClass<*>): List<Map<String, Int>> =
     clazz.constructors
         .map { constructor ->
             constructor.parameters
                 .mapNotNull { it.name }
                 .mapIndexed { i, v -> v to i }
                 .toMap()
         }
+
+/**
+ * Sorts [this] according to the order of them in the constructors of [klass].
+ * It prefers the primary constructor if it exists, else it falls back to the other constructors to do the sorting.
+ * Finally, it falls back to lexicographical sorting if a property does not exist in any constructor.
+ */
+internal fun <T> Iterable<KCallable<T>>.sortWithConstructors(klass: KClass<*>): List<KCallable<T>> {
+    require(all { it.isGetterLike() })
+    val primaryConstructorOrder = getPropertyOrderFromPrimaryConstructor(klass)
+    val allConstructorsOrders = getPropertyOrderFromAllConstructors(klass)
+
+    // starting off lexicographically, sort properties according to the order of all constructors
+    val allConstructorsSortedProperties = allConstructorsOrders
+        .fold(this.sortedBy { it.columnName }) { props, constructorOrder ->
+            props
+                .withIndex()
+                .sortedBy { (i, it) -> constructorOrder[it.columnName] ?: i }
+                .map { it.value }
+        }.toList()
+
+    if (primaryConstructorOrder == null) {
+        return allConstructorsSortedProperties
+    }
+
+    // prefer to sort properties according to the order in the primary constructor if it exists.
+    // if a property does not exist in the primary constructor, fall back to the other order
+
+    val (propsInConstructor, propsNotInConstructor) =
+        this.partition { it.columnName in primaryConstructorOrder.keys }
+
+    val allConstructorsSortedPropertyNames = allConstructorsSortedProperties.map { it.columnName }
+
+    return propsInConstructor.sortedBy { primaryConstructorOrder[it.columnName] } +
+        propsNotInConstructor.sortedBy { allConstructorsSortedPropertyNames.indexOf(it.columnName) }
+}
diff --git a/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/Utils.kt b/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/Utils.kt
@@ -24,3 +24,4 @@ fun <T : DataFrame<*>> T.alsoDebug(println: String? = null, rowsLimit: Int = 20)
     print(borders = true, title = true, columnTypes = true, valueLimit = -1, rowsLimit = rowsLimit)
     schema().print()
 }
+
diff --git a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt
@@ -122,7 +122,7 @@ public interface TraversePropertiesDsl {
 
     /**
      * Skip given [properties] during recursive (dfs) traversal.
-     * These can also be getter-like functions.
+     * These can also be getter-like functions (like `getX()` or `isX()`).
      */
     public fun exclude(vararg properties: KCallable<*>)
 
@@ -133,7 +133,7 @@ public interface TraversePropertiesDsl {
 
     /**
      * Store given [properties] in ValueColumns without transformation into ColumnGroups or FrameColumns.
-     * These can also be getter-like functions.
+     * These can also be getter-like functions (like `getX()` or `isX()`).
      */
     public fun preserve(vararg properties: KCallable<*>)
 }

diff --git a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Utils.kt b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Utils.kt
@@ -17,13 +17,15 @@ import java.math.BigInteger
 import kotlin.reflect.KCallable
 import kotlin.reflect.KClass
 import kotlin.reflect.KFunction
+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
 import kotlin.reflect.full.starProjectedType
+import kotlin.reflect.full.valueParameters
 import kotlin.reflect.full.withNullability
 import kotlin.reflect.jvm.jvmErasure
 import kotlin.reflect.typeOf
@@ -338,8 +340,22 @@ internal fun List<String>.joinToCamelCaseString(): String {
         .replaceFirstChar { it.lowercaseChar() }
 }
 
+internal fun KFunction<*>.isGetterLike(): Boolean =
+    (name.startsWith("get") || name.startsWith("is")) && valueParameters.isEmpty()
+
+internal fun KCallable<*>.isGetterLike(): Boolean =
+    when (this) {
+        is KProperty<*> -> true
+        is KFunction<*> -> isGetterLike()
+        else -> false
+    }
+
 @PublishedApi
-internal val <T> KCallable<T>.columnName: String
+internal val KProperty<*>.columnName: String
+    get() = findAnnotation<ColumnName>()?.name ?: name
+
+@PublishedApi
+internal val KCallable<*>.columnName: String
     get() = findAnnotation<ColumnName>()?.name
         ?: when (this) {
             // for defining the column names based on a getter-function, we use the function name minus the get/is prefix
@@ -349,5 +365,7 @@ internal val <T> KCallable<T>.columnName: String
                     .removePrefix("is")
                     .replaceFirstChar { it.lowercase() }
 
+            is KProperty<*> -> this.columnName
+
             else -> name
         }