To dataframe improvements #1081
Conversation
| @@ -65,6 +65,9 @@ object Names { | |||
| val LOCAL_DATE_CLASS_ID = kotlinx.datetime.LocalDate::class.classId() | |||
| val LOCAL_DATE_TIME_CLASS_ID = kotlinx.datetime.LocalDateTime::class.classId() | |||
| val INSTANT_CLASS_ID = kotlinx.datetime.Instant::class.classId() | |||
| val DATE_TIME_PERIOD_CLASS_ID = kotlinx.datetime.DateTimePeriod::class.classId() | |||
There was a problem hiding this comment.
I'll later remove dependency on kotlinx.datetime, please call ClassId constructor with Fqname + String (check examples above) :)
| ), session | ||
| ) || | ||
| this.isSubtypeOf( | ||
| ConeClassLikeTypeImpl( |
There was a problem hiding this comment.
let's refactor it a little, extract class ids in Names object and then call Names.TEMPORAL.constructClassLikeType instead of ConeClassLikeTypeImpl...
There was a problem hiding this comment.
Ok, btw why here was isNullable=false?
There was a problem hiding this comment.
Good catch! For number type i have two subtypeOf, one with nullable type and other with non-nullable. For Temporal i missed one. But 1 check with nullable = true will be enough and it's more correct. Test case that works incorrectly now:
class Test(val temporal: Temporal?)
listOf(Test(LocalDateTime.parse("01/01/08 00:02:04", DateTimeFormatter.ofPattern("MM/dd/yy HH:mm:ss"))), Test(null)).toDataFrame(maxDepth = 2)
| @@ -38,20 +39,37 @@ import kotlin.reflect.jvm.javaField | |||
| import kotlin.reflect.typeOf | |||
|
|
|||
| private val valueTypes = setOf( | |||
| Any::class, | |||
There was a problem hiding this comment.
Uhm, Any doesn't cause issues with other classes? I guess this doesn't mean "a subtype of", rather the exact type? Maybe mention this in kdoc for future us
There was a problem hiding this comment.
User can provide Iterable<Any?>.
There was a problem hiding this comment.
And yeah, these are exact types.
There was a problem hiding this comment.
so now if a user has something like listOf(cat, dog, house, table).toDataFrame() they will get a value column, right?
core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt
Outdated
Show resolved
Hide resolved
sad we cannot have this for arrays :/ maybe mark the original issue as "won't fix" or "impossible" or something |
But as @koperagen said we don't need it since we have plugin ! |
| */ | ||
| @PublishedApi | ||
| internal val KClass<*>.hasProperties: Boolean | ||
| get() = this.memberProperties.isNotEmpty() |
There was a problem hiding this comment.
This might cause issues with POJO's, java objects with getters+setters instead of memberProperties. We support those with properties(). Maybe you can reuse some of the logic there
# Conflicts: # plugins/kotlin-dataframe/src/org/jetbrains/kotlinx/dataframe/plugin/impl/api/toDataFrame.kt # plugins/kotlin-dataframe/src/org/jetbrains/kotlinx/dataframe/plugin/utils/Names.kt
Iterable<ArrayLike>.toDataFrame()#676Fixed behavior for primitive arrays (due to compiler limitations, it is impossible to make extensions for primitive arrays), added extensions for common value types.