To dataframe improvements #1081
Conversation
| 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)
koperagen
left a comment
koperagen
left a comment
There was a problem hiding this comment.
Add a test for compiler plugin part covering the new case
Jolanrensen
left a comment
Jolanrensen
left a comment
There was a problem hiding this comment.
very nice stability and consistency improvements!
| 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.