Update compiler plugin #832
Conversation
koperagen
added
the
Compiler plugin
There was a problem hiding this comment.
Nice to have a lot more functions supported in the plugin :) I just have a few small comments, mostly regarding clarification of the new toDataFrame overload, otherwise, it's great. Could you mention which new functions are now supported in the PR notes? That'll help us find it back later
| @@ -209,7 +213,7 @@ public fun DataFrame.Companion.readExcel( | |||
| * @param range comma separated list of Excel column letters and column ranges (e.g. “A:E” or “A,C,E:F”) | |||
| */ | |||
| @JvmInline | |||
| public value class StringColumns(public val range: String) | |||
| public value class StringColumns @Interpretable("StringColumns") constructor(public val range: String) | |||
There was a problem hiding this comment.
The linter fails here, it expects it to be like:
@JvmInline
public value class StringColumns
@Interpretable("StringColumns")
constructor(public val range: String)I recommend using the KtLint plugin (if you run the IDE in K1 mode) or run the ktlint task manually
| @@ -151,6 +151,19 @@ df.add("length") { value.length } | |||
|
|
|||
| <!---END--> | |||
|
|
|||
| Creates a [`DataFrame`](DataFrame.md) with one column | |||
| made from [`Iterable`](https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/-iterable/) of values: | |||
There was a problem hiding this comment.
It took me a while to figure out how this case is different from the other cases. I'd specify a) that this uses non-basic types, since we have overloads for those already, and b) that by specifying the column name, the properties of the given objects aren't unfolded like in the case below. This should probably also be in kdocs next to the function
There was a problem hiding this comment.
Creates a DataFrame from Iterable<T> with one column: "columnName: DataColumn<T>"
Is it better?
There was a problem hiding this comment.
Yes! But probably also specify that the properties are not unfolded, aka, you get one "value column" and not a "column group".
| @@ -436,6 +436,8 @@ internal fun guessValueType(values: Sequence<Any?>, upperBound: KType? = null, l | |||
| collectionClasses.add(it.javaClass.kotlin) | |||
| } | |||
|
|
|||
| is Function<*> -> classes.add(Function::class) | |||
There was a problem hiding this comment.
why not :) Maybe we should change the rendering for functions in dataframes though. After a quick test I found it looks like:
⌌---------------------------------------------------------------------------------------------------------------⌍
| | a:Function<*>| b:Int|
|--|-----------------------------------------------------------------------------------------------------|------|
| 0| org.jetbrains.kotlinx.dataframe.testSets.person.DataFrameTests$$Lambda$60/0x000000010013a040@64ee819| 2|
⌎---------------------------------------------------------------------------------------------------------------⌏
There was a problem hiding this comment.
Sadly for such lambda objects toString is weird. I tried to look at the object in the debugger, but there's literally nothing that hints at signature or anything useful
There was a problem hiding this comment.
hmm you'd think there was a way in kotlin to detect it's a () -> Int or something :/
There was a problem hiding this comment.
It already renders correctly often!
might just be a fluke in the tests if the lambda is serialized as interface
There was a problem hiding this comment.
what kernel version do you use?
There was a problem hiding this comment.
I run the dev version of this PR's branch in the notebook. (so publish to maven local and use v=0.14.0-dev)
There was a problem hiding this comment.
Ah, ok, so the fix is needed anyway
| fun box(): String { | ||
| val df = dataFrameOf("a", "b")(1, null, null, "") | ||
| val df1 = df.fillNulls { b }.with { "empty" } | ||
| val b: DataColumn<String> = df1.b |
There was a problem hiding this comment.
this is beautiful behavior :) Literally how DataFrame is meant to be! Anatoly would be proud I'm sure
| val Arguments.separator: String by arg(defaultValue = Present(".")) | ||
|
|
||
| override fun Arguments.interpret(): PluginDataFrameSchema { | ||
| return receiver.asDataFrame().flatten(keepParentNameForColumns, separator).toPluginDataFrameSchema() |
There was a problem hiding this comment.
we can use the actual flatten function :D awesome!
|
oh btw, can we already update the kotlin version of the compiler plugin to 2.0.20-RC2? That's the latest now, the beta version is not available anymore |
| public fun <T, C> Update<T, C>.with(expression: UpdateExpression<T, C, C?>): DataFrame<T> = | ||
| @Refine | ||
| @Interpretable("UpdateWith0") | ||
| public fun <T, C, R : C?> Update<T, C>.with(expression: UpdateExpression<T, C, R>): DataFrame<T> = |
There was a problem hiding this comment.
In my testrun of the compiler plugin I now cannot use update {}.with {} anymore, just fillNulls {}.with {}. It gives
[NONE_APPLICABLE] None of the following candidates is applicable: val DataRow<Into_93I>.age: Int? val ColumnsContainer<Into_93I>.age: DataColumn<Int?>
when trying to access the updated column. Is this intended for now?
There was a problem hiding this comment.
For now yes, there's such issue because plugin fails to interpret update { }.with { } (update not supported) and fallback to an empty schema. Will fix
koperagen
force-pushed
the
compiler-plugin-member-functions
branch
from
327ec74
to
525d6b1
Compare
`with` used to have C? in UpdateExpression return type position, and so it was always inferred as nullable. Even for fillNulls { }.with { 123 }
…data-agnostic operations
From my quick research, reflection doesn't know anything about these values. They don't have invoke methods, nor any supertypes. So for now i decided to simply fix NPE by falling back to generic Function type for such columns. It will then at least work together with compiler plugin
koperagen
force-pushed
the
compiler-plugin-member-functions
branch
from
525d6b1
to
d86d589
Compare
|
Generated sources will be updated after merging this PR. |
No description provided.