-
Notifications
You must be signed in to change notification settings - Fork 76
Nothing cols fixes #1551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nothing cols fixes #1551
Conversation
…performance of allNulls() in some cases
…urate than `Unit` as getting something out of this column will always result in an exception, not "just run", like `Unit` does. Added emptyOf for if you need a specific type
…dded redundancy checks for nothing types in column creation
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/update.kt
Outdated
Show resolved
Hide resolved
AndreiKingsley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I've missed that behavior a few times.
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/DataColumn.kt
Outdated
Show resolved
Hide resolved
| * | ||
| * __NOTE:__ Null-filled columns of type [Nothing?][Nothing] will be included when selecting [`colsOf`][colsOf]`<T?>()`. | ||
| * This is because [Nothing][Nothing] is considered a subtype of all other types in Kotlin. | ||
| * To exclude these columns, call `.`[filter][ColumnsSelectionDsl.filter]` { !it.`[allNulls][DataColumn.allNulls]`() }` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why colsOf<T?> except colsOf<Nothing> is not an option here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing cannot be used as reified argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just have a look what we have to do to achieve typeOf<Nothing>() in dataframe XD
dataframe/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/TypeUtils.kt
Line 563 in fc5033c
| internal val nothingType: KType = typeOf<List<Nothing>>().arguments.first().type!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we would need to have a shortcut for that nothingCols() for instance, but I'm not sure it's common enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or, you know, they type
colsOf<T?>() except colsOf(typeOf<List<Nothing>>().arguments.first().type!!)you know, elegant
7d84d17 to
5b528f5
Compare
This PR contains a handful of fixes and optimizations regarding
Nothing(?)in column types:Nothing?column is detected asFormattedFrame<*>?in notebooks #1546 and adds a helpful note tocolsOfto prevent future confusionDataColumn.empty()created aUnit-typed column. I have no idea why, I made itNothing, which better represents thatemptyCol.get(0): Nothingwill always throw an exception. AddedDataColumn.emptyOf<T>()if you need a specifically typed empty column.updateprovided little info, so I addedUpdateExceptionfor this.::class-logic totypeOf's (for efficiency), and added redundancy checks:Nothingcolumn, it can only do that if it has no values; so the resulting column must be empty)Nothing?column, it can only do that if all its values arenull)I also checked usages of
colsOf()querying for nullable types to see if similar issues like #1546 existed. Fortunately, in most cases, including null-filled columns are not an issue! All statistics functions simply skip nulls, for instance.There are some unfortunate cases, like:
dataFrameOf( "a" to columnOf(1, 2, 3, null), "b" to columnOf(null, null, null, null), "c" to columnOf(7, 3, 2, 65), ).fillNulls { colsOf<Int?>() }.with { 0 }which throw
java.lang.IllegalArgumentException: Can not add value of class kotlin.Int to column of type kotlin.Nothing?. Value = 0because columnb: Nothing?is included bycolsOf<Int?>(). However, hopefully, the note to add.filter { !it.allNulls() }is found :). Alternatively, we could decide to add an argumentcolsOf<Int?>(exactType = true)in the future.