-
Notifications
You must be signed in to change notification settings - Fork 66
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
Extend "unfold" operation and support it in the compiler plugin #742
base: master
Are you sure you want to change the base?
Changes from all commits
5492921
abe7157
2cda967
18beb71
382d140
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,22 +5,25 @@ import org.jetbrains.kotlinx.dataframe.AnyColumnReference | |
import org.jetbrains.kotlinx.dataframe.ColumnsSelector | ||
import org.jetbrains.kotlinx.dataframe.DataColumn | ||
import org.jetbrains.kotlinx.dataframe.DataFrame | ||
import org.jetbrains.kotlinx.dataframe.columns.ColumnKind | ||
import org.jetbrains.kotlinx.dataframe.annotations.Interpretable | ||
import org.jetbrains.kotlinx.dataframe.annotations.Refine | ||
import org.jetbrains.kotlinx.dataframe.columns.toColumnSet | ||
import org.jetbrains.kotlinx.dataframe.impl.api.createDataFrameImpl | ||
import org.jetbrains.kotlinx.dataframe.typeClass | ||
import org.jetbrains.kotlinx.dataframe.impl.api.unfoldImpl | ||
import kotlin.reflect.KProperty | ||
|
||
public inline fun <reified T> DataColumn<T>.unfold(): AnyCol = | ||
when (kind()) { | ||
ColumnKind.Group, ColumnKind.Frame -> this | ||
else -> when { | ||
isPrimitive() -> this | ||
else -> values().createDataFrameImpl(typeClass) { | ||
(this as CreateDataFrameDsl<T>).properties() | ||
}.asColumnGroup(name()).asDataColumn() | ||
} | ||
} | ||
public inline fun <reified T> DataColumn<T>.unfold(vararg props: KProperty<*>, maxDepth: Int = 0): AnyCol = | ||
unfoldImpl(skipPrimitive = true) { properties(roots = props, maxDepth = maxDepth) } | ||
|
||
public inline fun <reified T> DataColumn<T>.unfold(noinline body: CreateDataFrameDsl<T>.() -> Unit): AnyCol = | ||
unfoldImpl(skipPrimitive = false, body) | ||
|
||
public inline fun <T, reified C> ReplaceClause<T, C>.unfold(vararg props: KProperty<*>, maxDepth: Int = 0): DataFrame<T> = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, we should also use |
||
with { it.unfold(props = props, maxDepth) } | ||
|
||
@Refine | ||
@Interpretable("ReplaceUnfold1") | ||
public inline fun <T, reified C> ReplaceClause<T, C>.unfold(noinline body: CreateDataFrameDsl<C>.() -> Unit): DataFrame<T> = | ||
with { it.unfoldImpl(skipPrimitive = false, body) } | ||
|
||
public fun <T> DataFrame<T>.unfold(columns: ColumnsSelector<T, *>): DataFrame<T> = replace(columns).with { it.unfold() } | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package org.jetbrains.kotlinx.dataframe.impl.api | ||
|
||
import org.jetbrains.kotlinx.dataframe.AnyCol | ||
import org.jetbrains.kotlinx.dataframe.DataColumn | ||
import org.jetbrains.kotlinx.dataframe.api.CreateDataFrameDsl | ||
import org.jetbrains.kotlinx.dataframe.api.asColumnGroup | ||
import org.jetbrains.kotlinx.dataframe.api.asDataColumn | ||
import org.jetbrains.kotlinx.dataframe.api.isPrimitive | ||
import org.jetbrains.kotlinx.dataframe.columns.ColumnKind | ||
import org.jetbrains.kotlinx.dataframe.typeClass | ||
|
||
@PublishedApi | ||
internal fun <T> DataColumn<T>.unfoldImpl(skipPrimitive: Boolean, body: CreateDataFrameDsl<T>.() -> Unit): AnyCol { | ||
return when (kind()) { | ||
ColumnKind.Group, ColumnKind.Frame -> this | ||
else -> when { | ||
skipPrimitive && isPrimitive() -> this | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was very confused, like how can you unfold a primitive? but it's an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can't. Have a look There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but can you take a look at the |
||
else -> values().createDataFrameImpl(typeClass) { | ||
body((this as CreateDataFrameDsl<T>)) | ||
}.asColumnGroup(name()).asDataColumn() | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
package org.jetbrains.kotlinx.dataframe.api | ||
|
||
import io.kotest.matchers.shouldBe | ||
import org.jetbrains.kotlinx.dataframe.DataRow | ||
import org.jetbrains.kotlinx.dataframe.io.readJsonStr | ||
import org.junit.Test | ||
import kotlin.reflect.typeOf | ||
|
||
|
@@ -13,4 +15,61 @@ class ReplaceTests { | |
conv.columnNames() shouldBe listOf("b") | ||
conv.columnTypes() shouldBe listOf(typeOf<Double>()) | ||
} | ||
|
||
@Test | ||
fun `unfold primitive`() { | ||
val a by columnOf("123") | ||
val df = dataFrameOf(a) | ||
|
||
val conv = df.replace { a }.unfold { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use replace and unfold independently? If somehow yes, could you please add test for this, of only together, could be combined to one function? |
||
"b" from { it } | ||
"c" from { DataRow.readJsonStr("""{"prop": 1}""") } | ||
} | ||
|
||
val b = conv["a"]["b"] | ||
b.type() shouldBe typeOf<String>() | ||
b.values() shouldBe listOf("123") | ||
|
||
val c = conv["a"]["c"]["prop"] | ||
c.type() shouldBe typeOf<Int>() | ||
c.values() shouldBe listOf(1) | ||
} | ||
|
||
@Test | ||
fun `unfold properties`() { | ||
val col by columnOf(A("1", 123, B(3.0))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this case "More fine-grained toDataFrame. Instead of converting 20-30 properties to 2-3 level of nesting all at once user can choose to convert toDataFrame(maxDepth = 0) and unfold required properties to whatever level they need" covered here, in this test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, yes. I intend to have a more representative example as a part of compiler plugin demo. There's a tree of objects with many properties and potentially deep nesting from konsist library. It will be a good illustration. But here it merely unfolds one specific column up to 2 levels. |
||
val df1 = dataFrameOf(col) | ||
val conv = df1.replace { col }.unfold(maxDepth = 2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not specifying maxDepth now breaks, while it worked before. |
||
|
||
val a = conv["col"]["a"] | ||
a.type() shouldBe typeOf<String>() | ||
a.values() shouldBe listOf("1") | ||
|
||
val b = conv["col"]["b"] | ||
b.type() shouldBe typeOf<Int>() | ||
b.values() shouldBe listOf(123) | ||
|
||
val d = conv["col"]["bb"]["d"] | ||
d.type() shouldBe typeOf<Double>() | ||
d.values() shouldBe listOf(3.0) | ||
} | ||
|
||
class B(val d: Double) | ||
class A(val a: String, val b: Int, val bb: B) | ||
|
||
@Test | ||
fun `skip primitive`() { | ||
val col1 by columnOf("1", "2") | ||
val col2 by columnOf(B(1.0), B(2.0)) | ||
val df1 = dataFrameOf(col1, col2) | ||
val conv = df1.replace { nameStartsWith("col") }.unfold() | ||
|
||
val a = conv["col1"] | ||
a.type() shouldBe typeOf<String>() | ||
a.values() shouldBe listOf("1", "2") | ||
|
||
val b = conv["col2"]["d"] | ||
b.type() shouldBe typeOf<Double>() | ||
b.values() shouldBe listOf(1.0, 2.0) | ||
} | ||
} |
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.
I think the name
unfolding
would read better, orbyUnfolding
/withUnfolded
.replace {}.unfold {}
doesn't read as a sentence anymore.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.
If it's possible, let's avoid motion or gravity to the native language, I believe, it's not a goal