forgot defaultValue in createColumnGuessingType(), removed column siz…

…e check from guessing type. May be unexpected behavior. Fixed tests
Kotlin · Oct 28, 2024 · 222086a · 222086a
1 parent dfaf46e
commit 222086a
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 48 deletions.
diff --git a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt
@@ -301,7 +301,7 @@ public fun dataFrameOf(header: Iterable<String>, values: Iterable<Any?>): DataFr
 
 public inline fun <T, reified C> dataFrameOf(header: Iterable<T>, fill: (T) -> Iterable<C>): DataFrame<*> =
     header.map { value ->
-        createColumnGuessingType(
+        DataColumn.createWithTypeInference(
             name = value.toString(),
             values = fill(value).asList(),
             suggestedType = TypeSuggestion.InferWithUpperbound(typeOf<C>()),
@@ -341,7 +341,7 @@ public class DataFrameBuilder(private val header: List<String>) {
 
     public inline operator fun <reified T> invoke(crossinline valuesBuilder: (String) -> Iterable<T>): DataFrame<*> =
         withColumns { name ->
-            createColumnGuessingType(
+            DataColumn.createWithTypeInference(
                 name = name,
                 values = valuesBuilder(name).asList(),
                 suggestedType = TypeSuggestion.InferWithUpperbound(typeOf<T>()),

diff --git a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/columns/TypeSuggestion.kt b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/columns/TypeSuggestion.kt
@@ -23,8 +23,7 @@ public sealed interface TypeSuggestion {
             when {
                 suggestedType != null && guessType -> InferWithUpperbound(suggestedType)
                 suggestedType != null && !guessType -> Use(suggestedType)
-                suggestedType == null && guessType -> Infer
-                else -> error("Cannot create TypeSuggestion with no suggested type and no guessing allowed.")
+                else -> Infer // no type was suggested, so we need to guess, no matter what guessType is
             }
     }
 

diff --git a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/TypeUtils.kt b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/TypeUtils.kt
@@ -392,7 +392,7 @@ internal fun <T> getValuesType(values: List<T>, type: KType, infer: Infer): KTyp
  * @param listifyValues if true, then values and nulls will be wrapped in a list if they appear among other lists.
  *   For example: `[1, null, listOf(1, 2, 3)]` will become `List<Int>` instead of `Any?`
  *   Note: this parameter is ignored if another [Collection] is present in the values.
- * @param allColsMakesRow if true, then, if all values are non-null same-sized columns, we assume
+ * @param allColsMakesRow if true, then, if all values are non-null columns, we assume
  *   that a column group should be created instead of a [DataColumn][DataColumn]`<`[AnyCol][AnyCol]`>`,
  *   so the function will return [DataRow].
  */
@@ -409,7 +409,6 @@ internal fun guessValueType(
     var hasFrames = false
     var hasRows = false
     var hasCols = false
-    val colSizes = mutableListOf<Int>()
     var hasList = false
     var allListsAreEmpty = true
     val classesInCollection = mutableSetOf<KClass<*>>()
@@ -423,10 +422,7 @@ internal fun guessValueType(
 
             is AnyFrame -> hasFrames = true
 
-            is AnyCol -> {
-                hasCols = true
-                colSizes += it.size()
-            }
+            is AnyCol -> hasCols = true
 
             is List<*> -> {
                 hasList = true
@@ -491,13 +487,7 @@ internal fun guessValueType(
         hasRows && !hasFrames && !hasList && !hasCols ->
             DataRow::class.createStarProjectedType(false)
 
-        allColsMakesRow &&
-            hasCols &&
-            !hasFrames &&
-            !hasList &&
-            !hasRows &&
-            !hasNulls &&
-            colSizes.distinct().size == 1 ->
+        allColsMakesRow && hasCols && !hasFrames && !hasList && !hasRows && !hasNulls ->
             DataRow::class.createStarProjectedType(false)
 
         collectionClasses.isNotEmpty() && !hasFrames && !hasRows && !hasCols -> {
@@ -509,24 +499,24 @@ internal fun guessValueType(
                 }
             }
             if (hasList) collectionClasses.add(List::class)
-            (commonParent(collectionClasses) ?: Collection::class)
-                .createTypeWithArgument(
-                    classesInCollection.commonType(
-                        nullable = nullsInCollection,
-                        upperBound = elementType ?: nothingType(nullable = nullsInCollection),
-                    ),
-                ).withNullability(hasNulls)
+            (commonParent(collectionClasses) ?: Collection::class).createTypeWithArgument(
+                argument = classesInCollection.commonType(
+                    nullable = nullsInCollection,
+                    upperBound = elementType ?: nothingType(nullable = nullsInCollection),
+                ),
+            ).withNullability(hasNulls)
         }
 
         hasList && collectionClasses.isEmpty() && !hasFrames && !hasRows && !hasCols -> {
-            val elementType = upperBound?.let { if (it.jvmErasure == List::class) it.arguments[0].type else null }
-            List::class
-                .createTypeWithArgument(
-                    classesInCollection.commonType(
-                        nullable = nullsInCollection,
-                        upperBound = elementType ?: nothingType(nullable = nullsInCollection),
-                    ),
-                ).withNullability(hasNulls && !listifyValues)
+            val elementType = upperBound?.let {
+                if (it.jvmErasure == List::class) it.arguments[0].type else null
+            }
+            List::class.createTypeWithArgument(
+                argument = classesInCollection.commonType(
+                    nullable = nullsInCollection,
+                    upperBound = elementType ?: nothingType(nullable = nullsInCollection),
+                ),
+            ).withNullability(hasNulls && !listifyValues)
         }
 
         else -> {

diff --git a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/constructors.kt b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/constructors.kt
@@ -239,7 +239,7 @@ internal fun <T> createColumnGuessingType(
 
     return when (type.classifier!! as KClass<*>) {
         // guessValueType can only return DataRow if all values are `AnyRow?`
-        // or allColsMakesColGroup == true, all values are `AnyCol`, and they all have the same size
+        // or allColsMakesColGroup == true, all values are `AnyCol`
         DataRow::class -> {
             if (allColsMakesColGroup && values.firstOrNull() is AnyCol) {
                 val df = dataFrameOf(values as Iterable<AnyCol>)
@@ -293,7 +293,12 @@ internal fun <T> createColumnGuessingType(
                 }
                 DataColumn.createFrameColumn(name, frames).cast()
             } else {
-                DataColumn.createValueColumn(name, lists, type, defaultValue = defaultValue).cast()
+                DataColumn.createValueColumn(
+                    name = name,
+                    values = lists,
+                    type = type,
+                    defaultValue = defaultValue,
+                ).cast()
             }
         }
 
@@ -310,6 +315,7 @@ internal fun <T> createColumnGuessingType(
                     // nullability already inferred by guessValueType
                     else -> Infer.None
                 },
+                defaultValue = defaultValue,
             )
     }
 }
diff --git a/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt b/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt
@@ -9,6 +9,7 @@ import org.jetbrains.kotlinx.dataframe.DataFrame
 import org.jetbrains.kotlinx.dataframe.DataRow
 import org.jetbrains.kotlinx.dataframe.columns.ColumnGroup
 import org.jetbrains.kotlinx.dataframe.columns.ColumnKind
+import org.jetbrains.kotlinx.dataframe.columns.TypeSuggestion.InferWithUpperbound
 import org.jetbrains.kotlinx.dataframe.impl.columns.createColumnGuessingType
 import org.jetbrains.kotlinx.dataframe.impl.nothingType
 import org.jetbrains.kotlinx.dataframe.type
@@ -49,7 +50,10 @@ class ConstructorsTests {
     @Test
     fun `guess column group from rows`() {
         val row = dataFrameOf("a", "b")(1, 2).single()
-        val col = createColumnGuessingType(listOf(row, DataRow.empty), typeOf<AnyRow>(), true)
+        val col = createColumnGuessingType(
+            values = listOf(row, DataRow.empty),
+            suggestedType = InferWithUpperbound(typeOf<AnyRow>()),
+        )
         col shouldBe columnOf(row, DataRow.empty)
 
         col.hasNulls() shouldBe false
@@ -62,7 +66,10 @@ class ConstructorsTests {
     @Test
     fun `guess column group from rows with null`() {
         val row = dataFrameOf("a", "b")(1, 2).single()
-        val col = createColumnGuessingType(listOf(row, DataRow.empty, null), typeOf<AnyRow?>(), true)
+        val col = createColumnGuessingType(
+            values = listOf(row, DataRow.empty, null),
+            suggestedType = InferWithUpperbound(typeOf<AnyRow?>()),
+        )
         col shouldBe columnOf(row, DataRow.empty, null)
 
         col.hasNulls() shouldBe false
@@ -79,8 +86,7 @@ class ConstructorsTests {
         val col2 = columnOf("a", "b")
         val col = createColumnGuessingType(
             values = listOf(col1, col2),
-            suggestedType = typeOf<AnyCol>(),
-            guessTypeWithSuggestedAsUpperbound = true,
+            suggestedType = InferWithUpperbound(typeOf<AnyCol>()),
             allColsMakesColGroup = true,
         )
         col shouldBe columnOf(col1, col2)
@@ -99,9 +105,8 @@ class ConstructorsTests {
         val col1 = columnOf(1, 2)
         val col2 = columnOf("a", "b")
         val col = createColumnGuessingType(
-            listOf(col1, col2, null),
-            typeOf<AnyCol?>(),
-            true,
+            values = listOf(col1, col2, null),
+            suggestedType = InferWithUpperbound(typeOf<AnyCol?>()),
         )
         col.values shouldBe columnOf(col1, col2, null).values
 
@@ -118,9 +123,8 @@ class ConstructorsTests {
         val df1 = dataFrameOf("a", "b")(1, 2)
         val df2 = dataFrameOf("a", "b")(3, 4)
         val col = createColumnGuessingType(
-            listOf(df1, df2, null),
-            typeOf<AnyCol?>(),
-            true,
+            values = listOf(df1, df2, null),
+            suggestedType = InferWithUpperbound(typeOf<AnyCol?>()),
         )
         col.values shouldBe columnOf(df1, df2, null).values
 
@@ -135,9 +139,8 @@ class ConstructorsTests {
     @Test
     fun `guess value column from nulls`() {
         val col = createColumnGuessingType(
-            listOf(null, null),
-            nothingType(true),
-            true,
+            values = listOf(null, null),
+            suggestedType = InferWithUpperbound(nothingType(true)),
         )
         col.values shouldBe columnOf<Any?>(null, null).values
 

diff --git a/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/types/UtilTests.kt b/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/types/UtilTests.kt
@@ -158,7 +158,7 @@ class UtilTests {
         guessValueType(
             sequenceOf(DataColumn.empty(), columnOf(1)),
             allColsMakesRow = true,
-        ) shouldBe typeOf<DataColumn<*>>()
+        ) shouldBe typeOf<DataRow<*>>()
 
         guessValueType(
             sequenceOf(columnOf("a"), columnOf(1)),